Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

Add click-able local storage items #31

Merged
merged 9 commits into from
Jun 19, 2018

Conversation

icyflame
Copy link
Member

@icyflame icyflame commented Apr 15, 2017

  • local storage is populated using the value of the query whenever the user clicked on a link
  • every item in the local storage is a clickable item

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:

image
image

Fixes #7

@icyflame icyflame changed the title Add local storage final Add click-able local storage items Apr 15, 2017
Copy link
Member

@athityakumar athityakumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works. 👍

From a UX point of view, wouldn't it be better if the last searched query was on the top of the "Previous searches" list?

@icyflame
Copy link
Member Author

@athityakumar Yeah, it would. Thanks for pointing it out! I will reverse chronological order is the one that makes sense.

@icyflame
Copy link
Member Author

@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);
Copy link
Member

@athityakumar athityakumar Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 32 of client.js

Won't localStorage.setItem("searched", query_value + "," + localStorage.getItem("searched")) do the job of chronological order?

Ping @icyflame

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@athityakumar
Copy link
Member

@icyflame - We should also probably add an issue for the mobile responsiveness, which we can add later. 😄

@icyflame
Copy link
Member Author

@athityakumar I don't understand, do you think there's something wrong with the mobile view?

@athityakumar
Copy link
Member

@icyflame - Nothing wrong, but it just doesn't look that awesome to me on mobile screen resolution.

image

Copy link
Member

@athityakumar athityakumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icyflame - Thanks for making local storage work 🎉 . I think it's high time we merge this. What say?

@icyflame
Copy link
Member Author

@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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no local-storage.js file being used. I'll fix this along with UI part. Sending a PR in an hour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, rebase onto the latest master. Thanks! 👍

@athityakumar
Copy link
Member

@icyflame - Can you review icyflame#2 please? After that is merged, we can merge this too. 😄

@icyflame
Copy link
Member Author

Hey @athityakumar, thanks for pointing me there. I will review and merge that. This is WAY over due! 😄

@icyflame
Copy link
Member Author

@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

image

Preview on a phone

screenshot_20171117-145921

@icyflame
Copy link
Member Author

icyflame commented Dec 9, 2017

Hey @amrav, slight nudge for a review. 🙂

@amrav
Copy link
Member

amrav commented Jan 8, 2018

@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 👍

@thealphadollar
Copy link
Contributor

@icyflame PR has been stagnant for a long time.

@ghostwriternr
Copy link
Member

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!

@icyflame
Copy link
Member Author

@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)

Final Demo

icyflame and others added 6 commits June 18, 2018 11:36
- 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
@icyflame
Copy link
Member Author

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.

@icyflame icyflame merged commit 9458d69 into metakgp:gh-pages Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants