Refactor the MediaReplaceFlow component to use Dropdown #19126
Conversation
youknowriad
added
the
[Type] Code Quality
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
| allowedTypes={ allowedTypes } | ||
| render={ ( { open } ) => ( | ||
| <Toolbar className={ 'media-replace-flow components-dropdown-menu' }> | ||
| <Dropdown |
There was a problem hiding this comment.
There is also this DropdownMenu component. Ou of curiosity, why isn’t it a good fit? We clearly need a ToolbarGroup which is collapsible and renders as button with menu. @diegohaz started some explorations.
There was a problem hiding this comment.
The reason it's not a DropdownMenu is because it contains elements that are not menu items (the URL input). There's some discussion about this in the original PR.
There was a problem hiding this comment.
Right, I'm personally wondering whether there should be any menu items used in this popover in that case. It might be a bit confusing for VoiceOver users.
|
@talldan @draganescu Any one able to test if this is still working as intended? |
|
Hi @youknowriad great PR, I tested this and it works nicely! I think this approach is better, can't say exactly why I settled on Popover but the flow has been through many iterations and (as you mentioned) some things required some refactoring of the core components at some stages of these iterations. In this stage a dropdown (not a dropdown-menu sinch this is more than a menu) is the perfect choice. There is a small difference in the roundness of the edit URL input:
In master the URL looks like this:
I am unsure if this is a problem. |
|
I can't reproduce the z-index issue. |
|
@youknowriad I tested in Firefox:
|
| // Changing the z-index of Popovers have wider implications. | ||
| .modal-open .block-editor-media-replace-flow__options { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Ah excellent solution! :)
|
@youknowriad the two problems are solved but now there is a too wide gap in the url editor between the input and the button.
versus
I think @mapk tweaked it quite a bit in the previous PR. |
Some code quality tweaks to the MediaReplaceFlow component.
I'd appreciate reviews to check whether the component is still working as intended.