Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update search page style #1265

Closed
wants to merge 1 commit into from
Closed

Conversation

fadihania
Copy link
Contributor

Fix having multiple vertical scrollbars while searching the icons, it's easier to use and better for User Experience

Description

Fix having multiple vertical scrollbars while searching the icons, it's easier to use and better for User Experience

Requirements / Checklist

What does this Pull Request (PR) do?

Fix scss styles

How should this be manually tested?

Same changes cap be applied using browser developer tools

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

image

@Finii
Copy link
Collaborator

Finii commented May 30, 2023

I totally agree! Thanks!

We need to see how this meshes with #1252.
BTW, what do you think about that PR? 😁

@Finii
Copy link
Collaborator

Finii commented May 30, 2023

Ah, the downside is for big results and especially if you start with 'all icons shown', that one does not really see/notice the comments at the bottom anymore:

image

@fadihania
Copy link
Contributor Author

fadihania commented May 31, 2023

Ah, the downside is for big results and especially if you start with 'all icons shown', that one does not really see/notice the comments at the bottom anymore:

image

I noticed that when updating that but that would be better than having double scrollbars next to each other, it's confusing in terms of usability. My suggestion would be is one of these options:

  • Move the comments either to the top or may be on large displays to the side
  • Reduce the vertical scroll by removing the unnecessary large search icon at the top and reduce the amount of empty vertical space. That way we can remove the outer scroll bar and fix the issue.
  • Having the footer/comments area (after removing the empty space) fixed at the bottom.

I prefer the third option, in addition to removing the large search icon at the top and maybe reduce the comment to a link to releases page where that is explained. Already those removed icons indicated using the badge on the top of each icon.

So, the idea is to remove one of those two scrollbars as they're kind of confusing

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2023

At the moment I struggle to get THIS working with the current (new) cheat-sheet - apart from placement of the extra info.
I tried to make the lazy load work with the full page, like so many one-pager websites these days, but did not manage fast enough. Maybe you are better with websites? Don't worry about the footer things for the time being, just the lazy load within the full-page scrollbar, that should be possible?

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2023

Reduce the vertical scroll by removing the unnecessary large search icon at the top

Well, but that is the unified design for all pages there. Changing this for the cheat sheet should change it throughout to keep the consistency - but that would be a major redesign and I would not touch that now.

@fadihania
Copy link
Contributor Author

Well, but that is the unified design for all pages there. Changing this for the cheat sheet should change it throughout to keep the consistency - but that would be a major redesign and I would not touch that now.

I agree too, this should be part of an overall website redesign.

I tried to make the lazy load work with the full page, like so many one-pager websites these days, but did not manage fast enough. Maybe you are better with websites? Don't worry about the footer things for the time being, just the lazy load within the full-page scrollbar, that should be possible?

Sure, I'll be glad to help, I'm a Web Developer.
I didn't get what you mean, do you mean to remove the inner icons scrollbar? Have the whole page including all icons displayed using a single scrollbar?

If so, that's the purpose of the PR I submitted is to fix that. Can you please explain more? Maybe a screenshot of the page after you applied the suggested CSS fixes

This is how it's supposed to look after removing those two CSS properties
image

Fix having multiple vertical scrollbars while searching the icons, it's easier to use and better for User Experience
@Finii
Copy link
Collaborator

Finii commented Jun 2, 2023

The cheat sheet code changed meanwhile. It loads only 250 icons and triggers to load another 250 if you scroll down (the inner thing). If we have no inner scrollbar that load-more-icons is never triggered.

I can change the code such that it does NOT lazy load the icons, and of course then there is no problem. But that is kind of bad because it now per default shows all icons (over 8000) ...

What I did not manage was to rewrite the new cheat-sheet code that the load-more-icons is triggered when the bottom of the page is shown/reached.

Hope this makes sense.

Force push to update this to the new cheat sheet code...

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2023

image

Scrolled down completely, and only the first 250 icons are shown. See how 'big' the scrollbar indicator is.
Lazy load is not triggered (of course, that is obvious from the code).
I do not know how to rewrite the code that it triggers on 'browser scroll down reached bottom of document' or how that is called :-)

Edit:

Search term is nothing (""), which will show all icons, which is the default when you enter the page the first time.
Previously the page started with no icons and only showed something when you entered a search term.
The problem is only relevant if the search results in more than 250 matches.

@Finii
Copy link
Collaborator

Finii commented Jun 2, 2023

Relevant code...

elementGlyphCheatSheet.onscroll = function () {
const threshold = 10;
if ((elementGlyphCheatSheet.offsetHeight + elementGlyphCheatSheet.scrollTop + threshold) >= elementGlyphCheatSheet.scrollHeight) {
console.log("load more search results");
loadMoreSearchResults();
}
};

@fadihania
Copy link
Contributor Author

fadihania commented Jun 3, 2023

I got you now, I didn't see the new change. So, you're accepting my changes but need the lazy load to work on the outer scrollbar after removing the inner one.

If so, I'll work on that later today and create another PR

@fadihania
Copy link
Contributor Author

fadihania commented Jun 4, 2023

I've already fixed that but need your opinion on this... What do you think is better?

  • Load the next patch on reaching the end of the page
  • Load the next patch on reaching the end of icons

Based on that I'll commit the changes and create new PR for you to review

@Finii
Copy link
Collaborator

Finii commented Jun 5, 2023

but need your opinion on this

Ugg. Well, if there is anything below the (currently loaded) icons - and only then the question seems to have any meaning - I guess you want to load further icons one you reach the end of the icons. If you load the icons only if you reach the bottom of the page the newly loaded icons will be somehow introduced in the middle of the currently shown part of the page. That might also work, but, I guess your 2nd option is more what I would call "natural". Meaning for the user it does not really show differently with or without lazy load (if the user scrolls slow enough).

I guess the difference is very subtle. Maybe only visible on very small screens (i.e. mobile).

and create new PR

Thank you, really appreciate

@Finii
Copy link
Collaborator

Finii commented Jun 5, 2023

I would move the "removed icons" and the "how to embed" stuff, that is now at the bottom, to some other place, so there is not much below the icon list anymore anyhow.

@fadihania
Copy link
Contributor Author

I've created a new PR #1286 with the new changes that includes scrollbars fix and lazy loading update based on that.

With regards to comments section at the bottom, I can move it but need to find a better place for that. That should also take into consideration the different screen sizes especially very small ones

Can you please have a look and let me know if that works as expected?

@Finii
Copy link
Collaborator

Finii commented Jun 8, 2023

Closing this in favor of #1286

@Finii Finii closed this Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants