-
Notifications
You must be signed in to change notification settings - Fork 330
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
Refactor into using a map for icon classes #454
Conversation
@Ionaru Please merge! |
Firstly, this is a project I maintain in my free time next to my day job. I look at all the issues and PRs, some take longer to look through and make a decision for. Please allow me the time for that. Secondly, I'm not seeing the use of this change. Sure it makes a neat little variable containing all icons, but ends up being more code for the exact same effect. I rather like the current look of the |
This PR serves as the underlying foundation for the next step. Which is: options.iconClassMap = extend({}, iconClassMap, options.iconClassMap || {}); Which allows integrators to easily use other icon sets without having to recreate every button themselves. |
Then build what you want to build and provide the completed thing as a PR. Smaller increments through PRs only increase the risk of half finished functionality ending up in the source code. |
Can this be merged? |
I would prefer the |
Alright, done. |
@willGabrielPereira This PR was never meant to let you change the icons, it was just a refactor and a stepping stone to later expose it as options. See the comment #454 (comment) above. |
No description provided.