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

[docs][material-icons] Add option to search for all icon styles #41415

Closed
wants to merge 8 commits into from

Conversation

cipherlogs
Copy link
Contributor

@cipherlogs cipherlogs commented Mar 9, 2024

Previously, the icon search selects the 'filled' style by default. This could lead to missing icons that existed in other styles but weren't shown in the search results unless you manually checked each style individually.

I've added an 'All' option to the search. With this new option, the search will include icons from all available styles, ensuring that no icons are missed due to being in a different style. Having the 'All' option available from the start makes it easier to find the desired icon without having to iterate through each style separately.

https://deploy-preview-41415--material-ui.netlify.app/material-ui/material-icons/

@mui-bot
Copy link

mui-bot commented Mar 9, 2024

Netlify deploy preview

https://deploy-preview-41415--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4246617

@danilo-leal danilo-leal added docs Improvements or additions to the documentation package: material-ui Specific to @mui/material labels Mar 9, 2024
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Functionality-wise, this makes a ton of sense to me 👍 Appreciate it!

@DiegoAndai
Copy link
Member

I wonder if this might impact SEO negatively, like #41126. What do you think @oliviertassinari?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 11, 2024

I think this change is SEO invariant, so not a concern. Concerns should be:

  • Page loading speed, it will be too slow, "All" can't be the default option
  • Algolia indexation, this will duplicate everything unless handled

UX-wise, this change is weird to me. The default option as "All" makes little, to no sense on my end. Applications style should be consistent, so first pick a variant. But maybe there is a need for a new All option. With Material Design v3, there is a notion of filled to signal that someone is active. https://fonts.google.com/icons?icon.set=Material+Icons&icon.style=Rounded doesn't support this filter.

I would close the PR or make it so that this "All" option is only available is there are less than 20 results. Otherwise, it's a footgun for performance and UI consistency.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 11, 2024
@danilo-leal
Copy link
Contributor

The default option as "All" makes little, to no sense on my end. Applications style should be consistent, so first pick a variant

Specifically about the UX part: That doesn't resonate much with me. I don't think that the "All" search option communicates the idea of inconsistency. I don't even think we should be that strict about it; that's for developers to decide, given their app's context. Also, it's pretty common for an app to use multiple icon styles. Take, for example, Google Drive's side nav, which is a widely used pattern: the selected item uses the filled icon, and all the other ones use the outlined.

Screenshot 2024-03-11 at 16 10 13

Having a way to quickly see how an icon looks in different styles would be handy for analyzing whether that icon looks good enough in both of them, particularly with the Material Icons, where there are a lot of them that look better in variant A vs. B. It's just a slightly more convenient way of doing this search without having a strong stance on how developers should use the icons and their available variants.

@DiegoAndai
Copy link
Member

Page loading speed, it will be too slow, "All" can't be the default option

I'm worried about this one as well. We could limit the number of icons we show for all styles. This would improve page loading speed for all. I don't think anyone is going through the 2,000+ icons list without searching anyway.

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

The latest changes where we don't see anything rendered by default is a UX regression in my opinion. If the "All" filter option costs that, then yeah, we might be better off without it 😕 We could think of adding some sort of empty state here, but then I still think the current experience (i.e., displaying icons sorted alphabetically) is better.

Screenshot 2024-03-13 at 09 11 34

@cipherlogs
Copy link
Contributor Author

In my opinion the search should aim to provide the most relevant icons upfront, reducing the user’s effort.

Instead of showing 500 matches, why not just show the top 30 most commonly used icons related to "x"? The user can quickly scan a set of 30 icons with their eyes. If they don't find what they're looking for, they can load more. Then, we'll show them the next 30 most relevant icons, and so on.

The number 30 is just an example. The point is that it doesn't make sense to overwhelm the user with tons of icons unless they specifically ask for more.

I've created a new draft that addresses a couple of minor changes. These changes make a big difference in how interactive the search feels. The idea is to return as few results as possible initially.

This approach is not related to the "Filter All" category. Other categories are expensive to render too.

Implementing infinite scrolling would be great. For now, I've just added a simple button that loads more icons when the user requests it.

Using Web Workers

The "Icon" component is expensive to render if a large list is used. Maybe we can offload that work to a Web Worker and keep the main thread free. However, in my opinion, why render all icons at once?

@cipherlogs

This comment was marked as off-topic.

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

I think we’re touching on a few, potentially independent topics:

  1. adding the “All” filter option, which is the PR focus
  2. returning the most relevant icons, which I’m not sure what we’d do differently here, so an issue would be great to clarify/discuss separately
  3. optimizations around rendering the Icon component
  4. the bug you mentioned there — go ahead on opening the issue!

I want to ensure this PR doesn’t side-track too much, and I feel like an issue for the “All” filter option would’ve also been nice before jumping into PR mode; we’d be able to discuss and come here more confidently.

In any case, the current deploy preview shows a somewhat degraded UX, with the results being loaded immediately as I type, causing a layout shift:

Before After
before after

@Janpot
Copy link
Member

Janpot commented Mar 15, 2024

Previously, the icon search selects the 'filled' style by default. This could lead to missing icons that existed in other styles but weren't shown in the search results unless you manually checked each style individually.

To solve this specific use-case differently we could also consider adding a count to each of the radio options?

[x] Filled (567)
[ ] Outlined (347)
[ ] Rounded (0)
...

That would make it instantly clear which style has icons for the search term and at no additional impact performance-wise.

The "Icon" component is expensive to render if a large list is used. Maybe we can offload that work to a Web Worker and keep the main thread free. However, in my opinion, why render all icons at once?

The bulk of the work is in rendering, i.e. in React, it has to happen on the main thread. #41330 (comment) solves it by deferring the rendering of the list and letting React prioritize the work so it can render certain state updates with higher priority. If not all the icons have to be on the page for SEO reasons, virtualizing this list is a good option.

@cipherlogs
Copy link
Contributor Author

cipherlogs commented Mar 15, 2024

we could also consider adding a count to each of the radio options?

Adding a count to each radio button is a good idea. We will need to filter all theme categories whenever there is a change, but that is not expensive. As the search gets more narrow, this becomes a good alternative approach.

The only potential downside is that I would have to switch from one theme to another just to see if I will like what is in that category, instead of viewing all options on one page.


#41330 (comment) solves it by deferring the rendering of the list

After reading your explanation, I still don't understand this part:

+ const deferredIcons = React.useDeferredValue(icons); 

icons is a list of objects created inside the component. It will cause unnecessary re-renders in the background because it will be different with every render when there is a new query.

I instead focused on the first triggers

query -> keys -> icons

Deferring the first trigger query helps a lot. Then deferring the second trigger keys makes things even better, because now icons uses the deferred keys to decide if it should update. I tried it and it worked well.

I also forced the Icon component to only re-render if there is a new set of icons or if the user requested to view more icons. Otherwise, it won't render again.

Because of these changes, icons is better controlled. For example, I removed the debounce wrapper around the query(0ms delay). This makes the input super responsive and I don't have to worry about keys and icons now.

Knowing if there is a new set of icons was extremely cheap (icons.length), as mentioned before, the search gets narrower as we type more characters.

-> air = 320 matches (render)

-> airplane = 50 matches (render)

-> airplan = 50 matches (no need to render)

-> airpla = 90 matches (render)

Rendering only when there is a new set keeps the input search and the whole experience smooth and fluid.

We can take this even further. Because the icons are sorted and because I have implemented virtualization to only show the top n icons, I can compare the head (beginning) and tail (end) of both the previous and next set to decide if we should re-render.

Relying only on length might have some edge cases. For example, imagine that "airplane" has 50 matches and "car" has 50 matches. If the user types "airplane", they will get the 50 matches. But if they select the whole word with 'Ctrl-a' and paste "car" instead with 'Ctrl-v', the update won't happen. However, this can easily be fixed.


If not all the icons have to be on the page for SEO reasons, virtualizing this list is a good option.

I understand what you're saying. I don't know if this is a valid option, but how about creating minimal and fast web pages for each icon? We could optimize each icon page with its synonyms, antonyms, and related/famous keywords. then load these pages in the website's sitemap specifically for search engines. It could help those pages rank much better in search results, leading to higher chances of people discovering mui.com

@Janpot
Copy link
Member

Janpot commented Mar 18, 2024

We will need to filter all theme categories whenever there is a change, but that is not expensive. As the search gets more narrow, this becomes a good alternative approach.

Most straightforward way to do this would probably be to just group icons by theme. And select out the icons for the current theme from the grouped icons and the lengths for the radio buttons.

icons is a list of objects created inside the component. It will cause unnecessary re-renders in the background because it will be different with every render when there is a new query.

In that setup icons is only updated when keys updates, and keys only updates after a debounced change of query. I may be wrong, but it seems to me that therefore icons changes at most as often as query, but likely fewer.

@ZeeshanTamboli
Copy link
Member

Any updates on the decision for this PR? I’ll hold off on sharing my opinion about adding this feature, but we should definitely consider improving the initial page load speed first (as discussed here). The search performance is already being addressed in #41330 (See #41330 (comment)).

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][material-icons] Add option to search for all icon styles [docs][material-icons] Add option to search for all icon styles Aug 9, 2024
@ZeeshanTamboli
Copy link
Member

Closing PR since it is inactive.

@oliviertassinari oliviertassinari added the status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: material-ui Specific to @mui/material PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants