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

Icon render slow depending on the word order in name property #2301

Closed
akomm opened this issue Nov 8, 2017 · 5 comments
Closed

Icon render slow depending on the word order in name property #2301

akomm opened this issue Nov 8, 2017 · 5 comments

Comments

@akomm
Copy link

akomm commented Nov 8, 2017

Looks like #2149 did not entirely fix #1184

Or is it not possible to "fix" the suggest performance via memoize entirely?

Steps

  1. Create Icon component
  2. Provide the name property "square check" (word order is important)
  3. (optional) Render it multiple times or copy & paste the same icon 10 times

Expected Result

Rendering should be fast.

Actual Result

Rendering is very slow. In my case 10 Icons under dev environment take about 600-800ms.
When you write "check square" it renders fast.

Did not test it with other multi-word icons.

Found the issue in suggest-usage from inside the semantic Icon component using performance profiling.

Version

0.76.0

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Nov 13, 2017

PR for this: #2314. I don't think memoize is enough, the lookup through an array is super expensive when there are multiple unique icons. I'm going to consider ways to optimize the word ordering, since that will always perform an expensive check, unfortunately.

@akomm
Copy link
Author

akomm commented Nov 14, 2017

I once noticed that the compiled CSS for icons (the one with content values) is quite large. So far it was not that critical and important, but I had the Idea to maybe create a small parser and go through FA variables.scss and generate an array of "icon name" => "content value", while "icon name" is split and ordered and joined together. Then create an Icon component, which accepts a name prop, but does split and order the words same way. The class/style for its content could be generated inside the Icon component itself. Ordering words consistent in Icon component and the precompilation of the name-value-array should make the need of searching and guessing obsolete.

Not sure how this fits into this project, as you have the CSS and this project actually separated, since the CSS for everything beside the content-values would still have to come from CSS and version matching might be important.

@brianespinosa
Copy link
Member

@akomm At some point in the future styles may become a part of this repository, but at present, we are relying on the structure of the CSS that comes out of the build tools for SUI core.

In my current project we are actually passing the icon names as the className instead for performance in development. As @levithomason mentioned on the original issue, this only effects dev environments. In production all PropType validation is not enabled.

@akomm
Copy link
Author

akomm commented Nov 15, 2017

Thanks for the post reference. I did find it too before I'v posted this issue. This is only for feedback purpose on the PR, which missed this edge case (word order). :)

@levithomason
Copy link
Member

This should be fixed in #2314. Release coming soon.

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

No branches or pull requests

4 participants