List View: Display lock status #40088
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
Mamaduka
commented
Apr 6, 2022
Mamaduka
added
[Feature] List View
|
Size Change: +99 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
|
Nice, looks great! The lock should inherit the white color when the item is selected. Looks like the block type icon on the left has some wrappers that gives this automatically, not sure if that's the best way to do it. Otherwise it's likely something involving |
|
@jasmussen, I just pushed a minor update to use a button with an icon instead of SVG. It should also fix the color inheritance issue + adds label. Since the icon does nothing, maybe we should choose a different method to communicate that block is locked. @alexstine, any suggestions? |
Could it open the same lock/unlock modal? |
Sure, but Block Options is right next to the lock status icon, and users could also use it to unlock. |
talldan
reviewed
Apr 6, 2022
Comment on lines
287
to
307
| { isLocked && ( | ||
| <TreeGridCell | ||
| className="block-editor-list-view-block__lock-cell" | ||
| aria-selected={ !! isSelected } | ||
| > | ||
| { ( { ref, tabIndex, onFocus } ) => ( | ||
| <Button | ||
| disabled | ||
| ref={ ref } | ||
| tabIndex={ tabIndex } | ||
| onFocus={ onFocus } | ||
| icon={ lock } | ||
| label={ sprintf( | ||
| /* translators: %s: block name */ | ||
| __( 'Locked %s' ), | ||
| blockInformation.title | ||
| ) } | ||
| /> | ||
| ) } | ||
| </TreeGridCell> | ||
| ) } |
There was a problem hiding this comment.
I think it can be simpler than making it a button in a cell, the cells should only really be used for focusable elements. An alternative would be to add it to the ListViewBlockSelectButton component as an Icon, after the anchor (https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/list-view/block-select-button.js). That component's children are laid out using flexbox, so hopefully the icon can be made to justify/align itself at the end. I think you'd hopefully then be able to use the similar CSS to the block icon to make it switch between black and white on block selection.
The other part that I think should be tackled is announcing the locked status to screenreaders.
I was having a look to see how the selected block is announced so that this could be done in the same way, but I can't see it in the code. There might have been a regression.
There was a problem hiding this comment.
Moving status icon into ListViewBlockSelectButton makes sense.
The other part that I think should be tackled is announcing the locked status to screenreaders.
You're right. Currently, locked blocks aren't announced. I am adding this issue to my to-do list. Thanks for checking and catching the issue, @talldan.
|
Moved lock status icon into Now it's just a wrapper span with an icon in it. I'm happy to work on the locked block announcements as a follow-up or in this PR. |
critterverse
approved these changes
Apr 6, 2022
|
Thanks for testing, @critterverse. I will wait for @talldan's code review for my recent changes and then merge it. |
talldan
approved these changes
Apr 7, 2022
alexstine
approved these changes
Apr 7, 2022
|
@alexstine, now that element isn't focusable, will aria-label work? @talldan, you mentioned making lock status part of the selected block announcement. The only announcement in the List View component is in Can you point me to the correct file, and I will do a follow-up. |
|
@Mamaduka I was thinking just making it part of the label, as Alex says. There used to be some visually hidden text in the I think you can do exactly the same visually hidden implementation for locked status. |
|
Thank you, @alexstine. So it's better to communicate lock status via The hidden text variant could be something like this: { isLocked && (
<VisuallyHidden>
{ __( '(locked block)' ) }
</VisuallyHidden>
) } |
|
@Mamaduka The link has |
|
Thanks again, @alexstine. I will start the new PR tomorrow. We can try and find the best option to include a label there. |
|
Ah, sorry for the bad advice, I'm a little outdated after recent changes to the component. Thanks for pointing us in the right direction, Alex! |
38 tasks
22 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.
What?
Resolves #40070.
Display block lock status in the "List View."
Why?
Lock status makes it easier to see which blocks are locked.
How?
Adds new
TreeGridCellonly displayed when the block is locked.P.S. I noticed that when one block is locked, the other block's content cells get different widths.
Testing Instructions
Screenshots or screencast