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

WIP: PoC for a media flow component in the block editor #16200

Open
wants to merge 27 commits into
base: master
from

Conversation

@draganescu
Copy link
Contributor

commented Jun 17, 2019

Description

Closes #11952

This component should wrap all media components and add the media edit functionality. It is uncertain if a HoC is a good idea or instead we should default to a MediaReplaceControl. I will try the control in a separate PR once the UX is working ok here.

This component, in order to work, had a few hurdles to handle:

  1. The is a z-index conflict that makes the suggestions from an url input appear below the popover when the popover is in a toolbar. The issue appears when the popover is displayed at the bottom and the problem is in _z-index.scss line 95, not sure what problem it solves there.

  2. The ObserveTyping component needed || target.closest( '.components-popover' ) to determine the typing is in the toolbar. There was a condition but now that we have a popover in the toolbar I found no other solution than to look for the popover container. Not sure this is the best way though.

  3. The LinkEditor component has some props forwarded to the URLInput it handles so we are able to show it with a border and full width.

  4. The DropdownMenu component needed two new props:

  • a showLabel prop that determines to show the label instead of the icon, when there is no icon
  • an onToggleHandler prop which is a function that is called by Dropdown on toggle. This was the only solid way to have the URLInput hidden when the menu was closed and opened again.
  • some style was added to have the arrow correctly spaced from the label as well
  1. The Toolbar component passes down showLabel and ... otherProps to DropdownMenu so we can set these in MediaFlow.
@jorgefilipecosta
Copy link
Member

left a comment

I like the idea of abstracting this flow, and it would make the life of block developers easier. Getting the shape of the right abstraction is a hard task; in our blocks, we have subtle differences in the flows. Let us try to use the MediaFlow component in all the media blocks, so we understand what can be abstracted and what cannot. Our blocks are a good sample of the needs block developers may have.

onSelect={ selectMedia }
onSelectURL={ selectURL }
accept="audio/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 16, 2019

Member

I guess this should be customized accept and allowedTypes should be customizable right?

This comment has been minimized.

Copy link
@draganescu

draganescu Jul 30, 2019

Author Contributor

yes, it was hardcoded for demo purposes :)

onSelectURL={ selectURL }
accept="audio/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }
/>

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 16, 2019

Member

Should we allow the user to pass functions that allow custom renders? For example for the cover and media text when the user selects an url we also will need that the user selects if the media is an image or a video. Maybe we can have a function that receives the current state (URL, selected media etc and renders a custom UI for cases like this).

This comment has been minimized.

Copy link
@draganescu

draganescu Jul 30, 2019

Author Contributor

the media flow component should not handle rendering. For the cover this component should only get the extended video + image accepted formats. Right?

@draganescu draganescu self-assigned this Jul 30, 2019

@draganescu draganescu force-pushed the add/media-flow-component branch from c2bc55e to dfddf2a Aug 2, 2019

@draganescu draganescu requested a review from chrisvanpatten as a code owner Aug 2, 2019

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I have changed this to fit closer UX wise to how it is described in #11952 but am still working on collecting an image URL from the replace image drop down.

The one thing where some technical feedback would be nice is this:

  • the dropdown menu closes itself when one clicks or focuses outside
  • the media upload component renders a specific thing: the button to open it. The url input does the same and allows for autofocus.
  • therefore the moment you mount/open something from the dropdown menu it unmounts and tears down the things you mounted from it.

Case in point, opening the media modal from a menu item will trigger the removal of the modal's contents (@gziolo you might know something about this and why it's needed).

To provent this, the dropdown should be allowed to stay open but on the other hand that means it may only be closed by clicking on the toggle handle again, not very nice.

What other ways are there?

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@mapk this is how it looks to use it so far, clicking the media library and the upload menu item do the job, I am still working on the URL inserter:

media-flow

@talldan

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

therefore the moment you mount/open something from the dropdown menu it unmounts and tears down the things you mounted from it.

Other modals are usually implemented like this:

{ isModalOpen && ( <Modal ... /> ) }

but this one is different because it's not a react modal and the component also has a render prop for the opening button. The only option I see with the current code is to render the Toolbar in the render prop for MediaUpload.

As an alternative that requires a bit of a refactor, the code for the modal could be extracted to work more like a normal modal. @jorgefilipecosta any thoughts on whether that would work?

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

It's looking amazing, @draganescu! I can't wait to see how the "Insert from URL" ends up.

One thing I noticed is the dropdown arrow and the word "Replace" needs more spacing to match the other icons that use a dropdown arrow. Here's an example of how it is in this PR:

Screen Shot 2019-08-19 at 11 06 26 AM

@youknowriad
Copy link
Contributor

left a comment

This is looking like a great start but I'm personally not convinced yet that we need a MediaFlow component. My concern is that for the sake of factorization, we'll add a lot of checks (if/else), props where it would have been ways simpler to just compose the inner components (MediaPlaceholder, MediaReplaceControl) in the block itself.

Maybe we can start by just adding a MediaReplaceControl to a specific block and then start thinking about genericity as we move towards implementing it in other blocks.

packages/components/src/dropdown-menu/index.js Outdated Show resolved Hide resolved

@draganescu draganescu force-pushed the add/media-flow-component branch from 6ecd0e7 to ce81ba7 Aug 20, 2019

@mtias

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Maybe we can start by just adding a MediaReplaceControl to a specific block and then start thinking about genericity as we move towards implementing it in other blocks.

I agree, I'd like to also see this flow in practice before an abstraction is in place and we extend it to every other block.

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Ok @mtias @youknowriad I will switch this to a MediaReplaceControl and implement that in the image block in a separate PR.

We have had the media flow component idea because of the fact that adding the current image editing flow to all the other media blocks proved a difficult and entangled process and we found that there was a lot of duplication between the media components.

On the other hand, the new UX is completely different so maybe having a MediaReplaceControl proves to be easier to distribute to all media blocks.

@draganescu draganescu force-pushed the add/media-flow-component branch from 6d2dfb2 to b1317dd Aug 22, 2019

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@mtias and @mapk take this for a spin and let me know how it feels. Currently all the actions work with the audio block on this PR. It is not exhaustively tested, just looking to know if the interaction is what you have in mind.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Just ran through another design review. I like how this is turning out! Here's some notes.

  1. The "Apply" button hover state needs to match the Image block.

hover

  1. When not focused, the input field is missing a border.

Screen Shot 2019-08-23 at 10 36 56 AM

  1. Clicking away from the popover should cause the URL input field to collapse again.
  2. The "Search" popover appears below the other popover. It should appear on top.

insert

@mapk mapk referenced this pull request Aug 23, 2019
5 of 5 tasks complete

@draganescu draganescu force-pushed the add/media-flow-component branch from 10540c1 to ad98c0f Sep 9, 2019

draganescu added 3 commits Sep 9, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Rebased and updated to show the URL in a click to edit fashion, like the link format in RichText.
@mapk take it for a spin and let me know :)

@mapk

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

Took it for a spin, and I like it! There only one minor request. When I've inserted media from a URL, and I go away, then come back to the block to edit it... when I click "Replace" I'd like to have the URL link visible right away without having to click the menu item to show it. This should only happen if there is a URL present. If there is no URL present, keep it hidden until the menu item is clicked.

current example

replaceimage

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

As an alternative that requires a bit of a refactor, the code for the modal could be extracted to work more like a normal modal. @jorgefilipecosta any thoughts on whether that would work?

It may be possible to abstract the media modal in a component and make it look like it is any other modal. Using effects when a component is mounted we would call js functions to open media modal when it unmounted we would close it. We may face some problems because the media library is not rendered in the dom like other modals (wp media js code controls the place in dom where it renders), and we may also face problems related with focus outside media library implements its own and it may collide with the one we use for editor modals. I'm not sure if right now we will have many benefits with this change.

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.