Deprecate duplicate icons #38849
Merged
Deprecate duplicate icons #38849
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not sure who to best ping for review here, so I've gone with GitHub's suggestion on who touched the code last. Also for awareness, CC: @iamthomasbishop |
7 tasks
|
Size Change: +353 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
dmsnell
reviewed
Feb 16, 2022
| /> | ||
| </SVG> | ||
| ); | ||
|
|
There was a problem hiding this comment.
might be worth moving the sparkles icon module from the icons package into this package. sure it's fine having the SVG defined inline here, but it's obviously generated/optimized and so practically un-editable in place and so a bit awkward in the middle of another component.
not a big issue and some may disagree with me on this, but I think keeping it an import from another file would make it easier to think about and maintain. we wouldn't have to call it sparkles even; we could give it a semantic name like block-is-new-icon.svg and then people would have the freedom to change it to something that isn't sparkles without changing the intent of the code/module/import.
There was a problem hiding this comment.
Thanks a bunch, that's much better. I took a stab at that in 499d4dc, hopefully I got the code right.
dmsnell
approved these changes
Feb 16, 2022
|
I'd really love some mobile eyes on this one before I land it. Thanks again for the review! |
fluiddot
requested changes
Feb 17, 2022
There was a problem hiding this comment.
I checked the icon difference and looks great to me 👍 . Not sure if @iamthomasbishop has any concern about this change, it would be great to have that design input.
| Before | After |
|---|---|
 |
 |
Apart from this, I added a comment for fixing the sparkles icon import as it's required to display it.
|
@fluiddot @jasmussen The icon changes look good to me! |
Co-authored-by: Carlos Garcia <[email protected]>
|
Thanks a whole lot for all the reviews. I applied the suggestion! 👌 |
gziolo
pushed a commit
that referenced
this pull request
Feb 23, 2022
* Remove alignJustifyAlt. * Remove cogAlt. * Remove sparkles, trashFilled. * Move to imported sparkles icon. * Update packages/components/src/mobile/inserter-button/index.native.js Co-authored-by: Carlos Garcia <[email protected]>
cbravobernal
added
the
[Type] Code Quality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I've been doing some cleanup work on the Figma WordPress Design library, notably the Icons page, and have noticed it's out of sync with the icons that are present in Storybook.
Notably, it contains a couple of new icons that should not be there, as they are not designed for the new icon set. It's important we only add icons we need to that set, as they get published as a npm package. If we have to use a different icon we need a good reason, and then we can add those icons locally, so they don't get published as part of the package.
justifyAltis removed,justifyis used insteadcogAltis removed,cogis used insteadsparklesis moved to be a local icon instead of part of the packagetrashFilledis removed,trashis used instead.Types of changes
This is a breaking change for the node package, so I've updated the changelog.
I haven't been able to test this in the mobile emulator, so it should not be merged without a green check of someone who's able to do that.
Checklist:
*.native.jsfiles for terms that need renaming or removal).