Format toolbar: visual clue for hidden active items #21892
Merged
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
ellatrix
requested review from
etoledom,
jorgefilipecosta and
SergioEstevao
as code owners
ellatrix
changed the title
|
Size Change: +37 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
|
My goodness, this feels like an excellent win! And serendipitously, I just discussed the other day on another ticket with @karmatosed exactly this treatment — what does a toolbar button look like when it has opened a submenu? In this case, obviously, the black "toggled" state does not indicate simply that a popover is open, but simply that a submenu item was toggled. I imagine this would benefit alignments too, do you think? But the remaining question is: what do we do with the indication that a submenu is open? Right now there's just a blue color:
Some options:
|
|
Yeah, do we need a visual clue for a submenu being open? In my opinion, the submenu appearing is enough of a visual clue. |
|
But I like extending this active visual clue to alignments, it makes sense. |
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
from
6669195
to
1ea5bc6
Compare
|
@jasmussen I added it for text alignment too. |
Cool! How about block alignments? |
|
Love this #21814 is the issue where I brought up the select state and maybe we can combine with this to close? |
|
Do we still need the "text color" button to show up separately with this change? |
|
@youknowriad What do you mean? :) |
|
If you select text, change it's color using the button in the drop-down, that color button now shows up among the other inline buttons instead of in the drop-down. With this visual indication, perhaps it can just stay in the drop-down. |
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
from
1ea5bc6
to
fbeaa23
Compare
|
@jasmussen @karmatosed How's this? :) |
|
It seems good to me:
The missing text color is my theme fault, not the code here. Did you want to move the inline text color indication as Riad suggested? Or is that a separate PR? |
|
Wasn't it but in the block toolbar for a reason? I didn't work on the text color button, so I don't know why it appears in the block toolbar when it's active. I assume so it's easy for people to know about the color? |
|
Aaah, or do you mean, now that the collapsed button indicates active state, we can move it back? |
Yes, essentially, because there's indication as to where you go to change it. There were also some challenges with the color button in the toolbar. |
|
I moved the active button, which is much better and more consistent imo. The color indicator is in the wrong position though. Here again I wonder... why is there a color indicator? You can see the color in both the text AND the color selection UI. |
|
Ok, I removed it. Ready for review. |
|
I think there is some value to showing the color swatch for the inline color. It's sort of a way to "augment the icon". I was just about to fix it, actually. |
|
|
|
I just tested this PR in Gutenberg.run and noticed a few issues. 1. Color indicator missing.The color bar is missing from the "A" icon. I should see a red bar in the screenshot below.
2. Off-centered iconThe dropdown arrow appears off-centered in the selected state of the icon.
3. Pressed state loses the iconWhen I press down on the dropdown icon, the icon disappears. Perhaps it is just displaying in black on a black background.
Tested on Chrome 81.0.4. |
Eagle eye, Mark! This is actually not a regression of this PR but is present in master:
But definitely good to fix of course. |
@mapk I removed the color indication because it doesn't make any sense to me there. The icons in the list should be consistent and there's color indication in both the text and the color panel that it opens. |
ellatrix
commented
May 5, 2020
| @@ -62,7 +62,7 @@ | |||
| background: transparent; | |||
| } | |||
|
|
|||
| &::before { | |||
| &:not(:active)::before { | |||
There was a problem hiding this comment.
@jasmussen Is this the right way? I'm not sure :)
There was a problem hiding this comment.
I can't immediately grok what it does. But my instinct was to simply add a separate :active modifier and make the text white there. But I can see that the following CSS would interfere with that:
.components-button:not([aria-disabled="true"]):active {
color: inherit;
}
So if what you have here works, I think it's good to ship. I don't know if it needs a comment.
Nice work.
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
from
67fce6e
to
f6af5a1
Compare
|
Is this good to go? I'll rebase. |
|
If the color indicator is back yes I think it's good 💯 |
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
from
f6af5a1
to
d2a9576
Compare
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
2 times, most recently
from
78d7fd4
to
6a5f10e
Compare
|
Is this one still relevant? If yes, then it needs to be rebased 👍 |
ramonjd
added
[Type] Enhancement
|
🎉 Agree that this one would be a nice win and great to land. Looks like it's ready to merge with a rebase @ellatrix? |
|
I agree with everyone. This would be a sweet improvement to land. |
ellatrix
force-pushed
the
try/rich-text-hidden-active
branch
from
6a5f10e
to
82cb606
Compare
7 tasks
|
Nice to see the closed loop :) |
7 tasks
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
How has this been tested?
Screenshots
Types of changes
Checklist: