Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the MediaReplaceFlow component to use Dropdown #19126

Merged
merged 6 commits into from Dec 16, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 13, 2019

Some code quality tweaks to the MediaReplaceFlow component.

I'd appreciate reviews to check whether the component is still working as intended.

allowedTypes={ allowedTypes }
render={ ( { open } ) => (
<Toolbar className={ 'media-replace-flow components-dropdown-menu' }>
<Dropdown

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 13, 2019

Member

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 16, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 16, 2019

Member

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.

youknowriad added 2 commits Dec 16, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 16, 2019

@talldan @draganescu Any one able to test if this is still working as intended?

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Dec 16, 2019

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:

Screenshot 2019-12-16 at 11 27 05

In master the URL looks like this:

Screenshot 2019-12-16 at 11 26 57

I am unsure if this is a problem.

Copy link
Contributor

draganescu left a comment

There are some small issues.

  1. The z-index of the dropdown is higher than the media library:

Screenshot 2019-12-16 at 11 32 46

  1. There is a weird second outline around the edit URL button:

Screenshot 2019-12-16 at 11 33 01

Other than this visual issues, functionality is the same for me.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 16, 2019

I can't reproduce the z-index issue.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Dec 16, 2019

@youknowriad I tested in Firefox:

media-replace-z-index

youknowriad added 2 commits Dec 16, 2019
// Changing the z-index of Popovers have wider implications.
.modal-open .block-editor-media-replace-flow__options {
display: none;
}

This comment has been minimized.

Copy link
@draganescu

draganescu Dec 16, 2019

Contributor

Ah excellent solution! :)

@youknowriad youknowriad requested a review from draganescu Dec 16, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Dec 16, 2019

@youknowriad the two problems are solved but now there is a too wide gap in the url editor between the input and the button.

Screenshot 2019-12-16 at 13 23 22

versus

Screenshot 2019-12-16 at 13 23 22

I think @mapk tweaked it quite a bit in the previous PR.

@youknowriad youknowriad merged commit 903df27 into master Dec 16, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/media-replace-flow branch Dec 16, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.