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

Allow icon sets to be filtered #84

Closed
wants to merge 12 commits into from

Conversation

joshhanley
Copy link

Hi @driesvints,

This is reference to issue #71 performance concerns, and implements your second suggestion to manually build a list of icons from a set.

I looked at caching the component names like the Laravel packages.php but decided it was to much to wrap my head around for today, so I thought filtering would be an easier option for now.

It took a bit of testing, as I didn't want to impact any of the existing icon packages, or require any updates/changes to them.

How this works is the user would need to publish the blade-icons.php config file and then add a list of icon names for the set they want to limit in the filters array. This also works using dot syntax for nested icons. Any sets not specified will have all their icons loaded, that way you can just limit specific sets.

Example: The below config filters the Blade Material Design Icons package as it contains over 5,000 icons

'filters' => [
        'blade-mdi' => [
            'menu',
            'home',
            'magnify',
            'chevron-right',
            'plus',
            'calendar-today',
            'calendar-range',
            'menu-down',
            'briefcase-variant',
            'account',
        ],

    ],

My test suite normally takes ~30secs but loading up the Blade Material Design Icons package meant I was getting times of ~1min40secs. After applying the above filter I'm back down to about ~30secs again.

I hope this helps. Let me know what you think and whether you think I need to change anything. I haven't updated the README or CHANGELOG yet, I will do so if you are happy with the way I've implmented this.

@driesvints
Copy link
Member

Hey @joshhanley. Thanks for the PR! I'm a bit strapped on time with the release of Blade UI Kit but I'm hoping to take a look at this soon. Could still be a while, sorry for that. Thanks for your work.

@joshhanley
Copy link
Author

@driesvints no worries! Congratulations on the launch. It looks really good!

@roni-estein
Copy link

Making this more efficient is definitely a needed improvement but instead of manually maintaining a list, having command that parsed your files on build would be a better step. We are already doing it for css classes, I'd imagine watching for blade icons would be similarly easy since there are less complications. you really need to only deal with <x-iconlibrary-something-until-a-space, I could see some issues with x-dynamic-component but lets say somethng like that could get you 99% of the way there. The adding A small uncaught whitelist might finisih it.

Honestly I'm coming from x-icon.my.files which I love, I just don't want to maintain over multiple projects.
This seems like a possible nice alternative without the performance hit.

@driesvints
Copy link
Member

Hi there. I'm sorry it took me this long to get to this. I'm happy to say that the next version of Blade Icons will include icon caching that will solve the existing performance problems. I'm currently looking for testers of the caching feature and would appreciate feedback on there PR here: #121 (comment)

In regard to that I'll be closing this PR in favor of the PR above. But I'd still very much would like to thank you for the effort you made in helping out here 👍

@driesvints driesvints closed this Mar 14, 2021
@joshhanley
Copy link
Author

@driesvints awesome, will test out when I get a chance! Thanks, and no worries 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants