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

Refactor into using a map for icon classes #454

Merged
merged 5 commits into from
Jun 16, 2022
Merged

Refactor into using a map for icon classes #454

merged 5 commits into from
Jun 16, 2022

Conversation

vanillajonathan
Copy link
Contributor

No description provided.

@vanillajonathan
Copy link
Contributor Author

@Ionaru Please merge!

@Ionaru
Copy link
Owner

Ionaru commented May 22, 2022

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 toolbarBuiltInButtons variable as it shows how users can add their own custom buttons.

@vanillajonathan
Copy link
Contributor Author

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.

@Ionaru
Copy link
Owner

Ionaru commented May 22, 2022

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.

@vanillajonathan
Copy link
Contributor Author

Can this be merged?

@Ionaru
Copy link
Owner

Ionaru commented Jun 16, 2022

I would prefer the iconClassMap to use the name property of the toolbar buttons because that is what the toolbar option uses as well.

@vanillajonathan
Copy link
Contributor Author

Alright, done.

@Ionaru Ionaru merged commit 4d065c3 into Ionaru:master Jun 16, 2022
@vanillajonathan vanillajonathan deleted the patch-8 branch June 16, 2022 16:07
@willGabrielPereira
Copy link

Hi @Ionaru, this PR was made for exactly what? because it keeps doing the same effect, you can't change the icons even after this changes.

The #501 looks like really fix this. Please take a look in it!

@vanillajonathan
Copy link
Contributor Author

@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.

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.

None yet

3 participants