Simplify the BlockBreadcrumb component #6507
Conversation
afercia
added
the
[Focus] Accessibility (a11y)
There was a problem hiding this comment.
Thank you for these improvements @afercia. This test well for me. I left a possible improvement that I think we can make to remove a wrapping div.
|
|
||
| return ( | ||
| <NavigableToolbar className={ classnames( 'editor-block-list__breadcrumb', { | ||
| <div className={ classnames( 'editor-block-list__breadcrumb', { |
There was a problem hiding this comment.
Instead of using a wrapping div can we pass the className directly in toolbar component? (Toolbar accepts a className prop).
There was a problem hiding this comment.
@jorgefilipecosta I've tried, but there's a lot of CSS involving also the block contextual toolbar. I can't make the positioning on desktop/mobile work nicely unless completely refactoring the CSS, which I'd like to avoid.
Worth noting with "wide" images, the breadcrumb completely hides the block movers (happens also on master) so I guess the breadcrumb styling will need some adjustments anyway:
There was a problem hiding this comment.
Hi @afercia, thank you for clarifying the usage of the div. It looks like in this case cannot avoid it without changes to the toolbar styles.
I made some tests and it looks like everything is working as expected 👍 Thank you for this PR.
This PR aims to simplify the BlockBreadcrumb component and remove some semantics and features that are not needed in this case. For more details, please refer to the related issue #6337
role="toolbar"(added by NavigableToolbar via NavigableMenu)No visual changes:
Fixes #6337