Skip to content

Refactor: move all icons to common components - #445

Merged
sheabunge merged 1 commit into
core-betafrom
icons/core
Aug 9, 2026
Merged

Refactor: move all icons to common components#445
sheabunge merged 1 commit into
core-betafrom
icons/core

Conversation

@ramiy

@ramiy ramiy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Split grouped icon files into single-icon files. This change consolidate every icon into src/js/components/common/icons/, one file per icon (no grouping).

Also, all the icons are now have the same type, and they all accept props to allow custom size and styling.

@ramiy
ramiy requested review from imantsk and sheabunge August 7, 2026 16:29
@ramiy ramiy changed the title Refactor: move all icons to commons components Refactor: move all icons to common components Aug 7, 2026
@ramiy

ramiy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

We can merge this to 3.10, but we don't have to. It can also wait for 4.0.

@sheabunge

Copy link
Copy Markdown
Member

Happy to include this in 3.10, but I am curious – what is the benefit of keeping each icon in its own file?

@ramiy

ramiy commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

This is the best practice in many React projects, each component in a separate file. Not sure why, I guess it's something to do with tree shaking when bundling (small icons files instead of one giant icons file) and maybe also for code splitting when dynamically loading some routes (importing specific icons instead of large icon files). But I'm not sure. I just know that this is the best practice.

@sheabunge

Copy link
Copy Markdown
Member

Interesting! I am unsure if tree shaking applies if we are grouping the icons contextually (only ever importing from ToolbarIcons in the Toolbar file), but happy to enforce this purely for consistency purposes.

@sheabunge
sheabunge merged commit 71f3f20 into core-beta Aug 9, 2026
9 checks passed
@sheabunge
sheabunge deleted the icons/core branch August 9, 2026 04:12
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.

2 participants