-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Site Logo: Remove "Reset" button icon #35434
Conversation
Size Change: -18 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
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.
Thanks for the quick follow-up. This tested well ✔️
it turns out that MediaReplaceFlow doesn't use the DropdownMenu component, and we don't get all styling out of the box.
I'm not sure we want to duplicate all those styles just for a single menu item.
Sounds reasonable to me. Having the Reset button in its own menu group would be nice, but not a necessity IMO. LGTM!
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.
Thank you for the reviews, @stacimc and @andrewserong. |
Description
Follow-up for #35372 (comment).
PR removes the reset button icon.
I tried group menu items as @jasmussen suggested. But it turns out that
MediaReplaceFlow
doesn't use theDropdownMenu
component, and we don't get all styling out of the box.I'm not sure we want to duplicate all those styles just for a single menu item.
How has this been tested?
Screenshots
Types of changes
Enhancement
Checklist:
*.native.js
files for terms that need renaming or removal).