Add click-able local storage items #31
Conversation
|
@athityakumar Yeah, it would. Thanks for pointing it out! I will reverse chronological order is the one that makes sense. |
|
@vivekiitkgp @amrav Please review this PR. I will merge this in a few hours, after a little bit more testing, I feel confident about it though. @athityakumar I have noted your suggestion, unfortunately, I can't work on them right now. I am adding an issue for now, and I will work on them later. I think we should push this out this exam session! |
client.js
Outdated
| // this is a result link | ||
| var query_value = $("#query").val().trim(); | ||
| if(localStorage.getItem("searched")) { | ||
| localStorage.setItem("searched", localStorage.getItem("searched") + "," + query_value); |
There was a problem hiding this comment.
Line 32 of client.js
Won't localStorage.setItem("searched", query_value + "," + localStorage.getItem("searched")) do the job of chronological order?
Ping @icyflame
There was a problem hiding this comment.
Only on the next reload of the page.
When the page loads, I get all the items from the LocalStorage and add an appropriate <ul></ul> with <li> elements. And then I simply append <li> elements to this list.
There was a problem hiding this comment.
Hmm, but the recent search does get appended to the list when a paper link is clicked right? Anyway, I think it'd be better if we can get started on the chronological order before merging this PR.
|
@icyflame - We should also probably add an issue for the mobile responsiveness, which we can add later. 😄 |
|
@athityakumar I don't understand, do you think there's something wrong with the mobile view? |
|
@icyflame - Nothing wrong, but it just doesn't look that awesome to me on mobile screen resolution.
|
e587ac4
to
e6ef301
Compare
There was a problem hiding this comment.
@icyflame - Thanks for making local storage work 🎉 . I think it's high time we merge this. What say?
1b356db
to
b4590c6
Compare
|
@athityakumar mobile preview still isn't good. that needs to be fixed. if you have time, can you get to it? |
index.html
Outdated
| </body> | ||
| </div> | ||
| </div> | ||
| <script src="local-storage.js"></script> |
There was a problem hiding this comment.
There's no local-storage.js file being used. I'll fix this along with UI part. Sending a PR in an hour.
There was a problem hiding this comment.
Great, rebase onto the latest master. Thanks! 👍
|
@icyflame - Can you review icyflame#2 please? After that is merged, we can merge this too. 😄 |
|
Hey @athityakumar, thanks for pointing me there. I will review and merge that. This is WAY over due! 😄 |
356c8cb
to
ee3f75f
Compare
|
@amrav This patch is finally done! Can you please review this? A live demo is available at https://icyflame.github.io/mfqp Preview on a computer
Preview on a phone
|
|
Hey @amrav, slight nudge for a review. 🙂 |
|
@icyflame hey sorry for the delayed review! Code LGTM, but I'm not a fan of the two column layout. Google solves this by showing recent searches in the search bar itself, on focus, which is a neat solution. Another possibility is to have a two row layout, with a horizontal (wrapped) list of recent searches just beneath the search bar. If you feel strongly about the current layout, I'm happy to merge as-is 👍 |
|
@icyflame PR has been stagnant for a long time. |
|
I'm with Vikrant on this too. This two column layout seems a bit weird and pushes focus to the side. I like the simplicity that we have with MFQP right now. If something like the way Google does it can be added for these recent searches though, that would be pretty neat! |
|
@ghostwriternr @amrav noted. I have fixed the layout to show the recent searches after the results. This doesn't disturb the present UI. Since the discussion has gone on for long enough and this change is pretty small (UI wise), I will merge this in tomorrow unless someone raises some problems with the code. (I have re-indented the code so the diff might be a bit too big) |
- clicking on the item will fill the query box and then execute a search
Unique search results with recent search on top
- this ensures no data is printed in the console - the undefined check ensures that there are no errors in the console
76bdb93
to
b29b66e
Compare
|
Force pushed to rebase to latest gh-pages. The links weren't working before (old mfqp json file). The diff isn't that big actually. It's easy to read too, so please do if you can. Merging tomorrow morning. |
The diff is slightly long because I have moved all the CSS and JS to separate files to make it easier to understand. Diffs for everything after the first commit should be easily understandable.
Earlier discussion was here: #12
Live demo: https://icyflame.github.io/mfqp/
Screenshots:
Fixes #7