Show block icon in contentOnly toolbar #64694
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
|
Size Change: -870 B (-0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
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. |
talldan
approved these changes
Aug 22, 2024
There was a problem hiding this comment.
Only thing that stands out to me is the grayed out icon:
Some feedback from @richtabor would be good on that. Otherwise this looks good to me. 👍
Yes, it shouldn't be gray. |
|
It's gray because it's a disabled button in this state. I'll look at changing it to not be a button so we can style it to be black while maintaining good html semantics. |
jeryj
force-pushed
the
add/icon-contentonly-toolbar
branch
from
c6a3b3d
to
5939e0e
Compare
jeryj
commented
Aug 27, 2024
| if ( hideDropdown ) { | ||
| return ( | ||
| <ToolbarGroup> |
There was a problem hiding this comment.
I removed the <ToolbarGroup /> from this and the switcher, as it will only ever have one item so doesn't feel semantically like a group... Also, it's already within a group.
There was a problem hiding this comment.
This makes the BlockSwitcher the only component in the block-editor-block-toolbar__block-controls toolbar the group without the inner group. Both BlockLockToolbar and BlockMover have group wrappers.
Should we evaluate their removal separately?
There was a problem hiding this comment.
Probably. I thought about this too. My guess is that it was a styling issue and not a technical one originally. But you're right that it should be evaluated separately. I'll add the ToolbarGroup in and put in another PR that removes the ToolbarGroups from those.
There was a problem hiding this comment.
Removed the ToolbarGroups. We'll see if it breaks anything :) #64909
Comment on lines
+183
to
+196
| const BlockIndicator = ( { icon, showTitle, blockTitle } ) => ( | ||
| <> | ||
| <BlockIcon | ||
| className="block-editor-block-switcher__toggle" | ||
| icon={ icon } | ||
| showColors | ||
| /> | ||
| { showTitle && blockTitle && ( | ||
| <span className="block-editor-block-switcher__toggle-text"> | ||
| { blockTitle } | ||
| </span> | ||
| ) } | ||
| </> | ||
| ); |
There was a problem hiding this comment.
I made this into its own component so it could be shared between the two places it's used. I'm tempted to fully extract it as I know I've ran into issues where I wanted to get the block indicator for a block but didn't because it wasn't going to be too much work.
| </ToolbarGroup> | ||
| <ToolbarButton | ||
| disabled | ||
| className="block-editor-block-switcher__no-switcher-icon" |
There was a problem hiding this comment.
I want to extract this. block-editor-block-switcher__no-switcher-icon is a code smell to me. It's not a switcher, it's just an indicator at this point, and it doesn't seem to have an instance where it switches states on the fly.
|
|
||
| // Since it's not clickable, though, don't show a hover state. | ||
| &, | ||
| .block-editor-block-icon.has-colors { | ||
| &:hover { |
There was a problem hiding this comment.
Matching the comment worked to keep this consistent.
| @@ -46,12 +46,12 @@ | |||
| } | |||
|
|
|||
| // Even when the block switcher does not have any transformations, it still serves as a block indicator. | |||
| .components-button.block-editor-block-switcher__no-switcher-icon:disabled { | |||
| .components-button.block-editor-block-switcher__no-switcher-icon[aria-disabled="true"] { | |||
There was a problem hiding this comment.
It wasn't applying the CSS until switching to aria-disabled.
richtabor
approved these changes
Aug 27, 2024
Good catch. Removed. |
Mamaduka
approved these changes
Aug 29, 2024
|
It seems that after this pull request, the block icon is duplicated in pattern overrides cc: @talldan |
|
Yep, I can also repro, I'll see if I can fix it. |
This was referenced Sep 4, 2024
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.
Fixes #64663
What?
The block toolbar doesn't have any context when in contentOnly mode. The block icon would be helpful.
Why?
Give a landmark to the user.
How?
Adds contentOnly to the allowed modes that will show the icon.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast