-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Deprecate moreHorizontalMobile. #71172
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
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: 0 B Total Size: 1.92 MB ℹ️ View Unchanged
|
Flaky tests detected in 8e1d5f9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16994828961
|
Thanks for the PR! We might need to discuss the backward compatibility policy for the Personally, I think breaking changes are acceptable for standalone packages that aren't related to WordPress updates, as long as the changelog entries are updated appropriately. That is, breaking changes that consumers might only encounter if they explicitly bump the library version. cc @WordPress/gutenberg-components |
Thank you for the broader ping, it's an important one to get right. I also lean towards this breaking change being acceptable because of the specific subject matter (the icon being added in error), but an alternative that maintains back compat would be to export the same moreHorizontal icon also as moreHorizontalMobile. |
This might be the best approach for now. It's similar to the approach taken in #71172, which means we just need to make the following changes:
// ...
export { default as more } from './library/more';
export {
/** @deprecated Import `moreHorizontal` instead. */
default as moreHorizontalMobile,
default as moreHorizontal,
} from './library/more-horizontal';
export { default as moreVertical } from './library/more-vertical';
// ...
const {
Icon: _Icon,
// Deprecated aliases
warning: _warning,
moreHorizontalMobile: _moreHorizontalMobile,
...availableIcons
} = icons; |
I'm happy to push that change (but also feel free to if you already have the code written). I'd still appreciate input from the broader ping group you alerted, just to understand what best practices might be in place in this space. |
I made that PR, and to be perfectly honest I did not know at the time that |
At most, we might want to keep a note in the index file to not add an icon with that same name in the future. |
Thanks for the feedback, both. However the precise next step is unclear to me, so just to be sure: do I continue with full deprecation, but add a note in the index file to not add an icon with the same name? Or do I add the proposed back-compat code to export the same icon in two names, and add the note in the index file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good to merge as is. It's probably unlikely that something with the exact name moreHorizontalMobile
will be added in the future 😄
Thank you folks! In any case I'll stay subscribed to this one and keep an eye on any potential followups. |
What?
There exists two icons for the same:
These two:
The former is the WordPress icon style, and is designed to match the other icons in the set. The latter was added, best I can tell, to match older mobile OS system icons.
This PR removes and deprecates this icon, and suggests you use the moreHorizontal icon instead.
Why?
There are a few problems with the mobile variation:
The benefit of keeping this along and maintaining it is therefore unclear. Removing the icon should reduce the confusion, make the React native version be coherent with the rest of the system, and reduce maintenance burden of the icons.
How?
It's unclear to me how we can deprecate icons in this manner, but my belief is that the approach taken here is valid insofar as:
If a third party consumer relies on this icon, as they update the icon packages it should be a search and replace to fix the change.
Testing Instructions