Global Styles: Move the 'Edit colors' button to a standard menu item#36842
Merged
noisysocks merged 3 commits intotrunkfrom Dec 13, 2021
Merged
Global Styles: Move the 'Edit colors' button to a standard menu item#36842noisysocks merged 3 commits intotrunkfrom
noisysocks merged 3 commits intotrunkfrom
Conversation
|
Size Change: +30 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
Contributor
Author
|
The styling of the disabled menu is not quite right, but will fix that tomorrow if there is agreement to go ahead with this interim fix |
Contributor
|
Thanks for working on this fix, @glendaviesnz!
Agree, and there's also something weird with the width of the items:
Also, if you open the menu and there are no colors, the option to remove them is enabled. However, it's a bit weird to open a menu and find that none of the options can be clicked, so I wondered if we could disable the ellipsis in that initial state. |
86788d7 to
5b7c67b
Compare
Contributor
Author
|
@javierarce there isn't currently a disable prop on that dropdown component, but just removing the menu when not needed seems to provide a reasonably smooth experience:
What do you think about that approach? Have pushed this to the PR if you want to test it. |
Contributor
|
This looks good to me. The styling looks good to me, and I quite like removing the ellipsis when there are no custom colors. I think this makes a lot of sense as an interim fix; definitely a big improvement. |
7a65672 to
28f1581
Compare
ramonjd
reviewed
Dec 12, 2021
| onClose(); | ||
| } } | ||
| className="edit-palette-menu-button" | ||
| > |
Member
There was a problem hiding this comment.
Would it be possible to make this CSS class name more consistent with others in the component, e.g., components-palette-edit__menu-button
Contributor
Author
There was a problem hiding this comment.
good point - have made this change
ramonjd
reviewed
Dec 12, 2021
ramonjd
approved these changes
Dec 12, 2021
Member
ramonjd
left a comment
There was a problem hiding this comment.
This works as advertised and, as others have noted, is already a nice improvement.
I just added some notes about rejigging the class name to match the component's existing naming.
db7de8a to
a388d9e
Compare
Contributor
Author
|
Thanks for the review, @ramonjd, I have changed the class name as suggested. |
noisysocks
pushed a commit
that referenced
this pull request
Dec 13, 2021
…36842) * Move changes to the new palette edit component * Remove redundant style * Change class name to match existing Co-authored-by: Glen Davies <glen.davies@a8c.com>
57 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Moves the
Edit colorsbutton from the ellipsis to a standard menu item to reduce confusion around the UI for editing custom colors.Immediate fix for #36519 ahead of wider redesign of this panel.
How has this been tested?
Remember, as noted in #36519 this is not intended to be the full and final fix, but a quick update to reduce confusion ahead of more detailed UX fixes.
Screenshots
Before:
After: