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

Expand the new replace image flow out to other blocks #14795

Closed
6 tasks
kjellr opened this issue Apr 3, 2019 · 12 comments
Closed
6 tasks

Expand the new replace image flow out to other blocks #14795

kjellr opened this issue Apr 3, 2019 · 12 comments
Assignees
Labels
[Feature] Blocks Overall functionality of blocks [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@kjellr
Copy link
Contributor

kjellr commented Apr 3, 2019

#14142 added an improved flow for replacing an image:

swap

As per @mapk's note in the thread, this flow should be rolled out to other areas of the UI. Here's a quick list:

In the following blocks, the edit button in the toolbar opens the media modal directly, so they just need the pencil icon swapped out for the new "Replace" icon:

  • Media & Text
  • Cover
  • File

*In the case of the Embed blocks (and maybe Video + Audio?) showing the preview in the placeholder may be too complicated. If that's the case, it may be fine to leave that part out.

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Apr 3, 2019
@mapk
Copy link
Contributor

mapk commented Apr 4, 2019

I agree with your evaluation there. Let's remove the "Needs design feedback" and move toward a PR.

@mapk mapk added Needs Dev Ready for, and needs developer efforts and removed Needs Design Feedback Needs general design feedback. labels Apr 4, 2019
@talldan
Copy link
Contributor

talldan commented Apr 5, 2019

There's also #14788 for the Media & Text block. @draganescu might have already started.

@draganescu
Copy link
Contributor

Hi everyone, I have indeed already started! :) Will be posting a PR soon!

@draganescu
Copy link
Contributor

draganescu commented Apr 10, 2019

I have added a WIP PR #14918 that implements the new flow to video, audio, media, cover, file. I'll check embeds asap :)

@draganescu
Copy link
Contributor

Closed the #14918 PR and opened six instead:

@paaljoachim
Copy link
Contributor

paaljoachim commented May 27, 2019

The edit icon is then being replaced in a lot of blocks that deal with images or files.
The new icon shows the swapping/changing of a file or image.

I know of one more that might benefit from the swap icon and that is the edit gallery (pencil) icon.
In general in a gallery one might want to change an image with another image. With the RC just released today one can click the image in the gallery and click the x to remove the image or click edit icon to through the media module remove an image.

That means the main reason to click the edit gallery would be to add a new image.
If most/all other blocks that deal with images/files have the icon replaced one might as well keep the consistency with the gallery block. As they all have to do with making a change to the existing image or file.

Or we just leave the gallery block as it is and see if anyone asks about this at a later point.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 7, 2019
@afercia
Copy link
Contributor

afercia commented Jun 8, 2019

With regards to the problem that the "swap" (Edit) button disappears (focus loss) and may re-appear in a different position, see:
#16048
#15518 (review)

The first GIF posted by @kjellr makes sense to me and would allow to solve the problem:

  • the button stays
  • it works like a "toggle" to switch between preview and edit mode
  • it communicates its "pressed" state

Seems to me this should apply to all the blocks that switch between the preview and the placeholder.

I quickly tried it on the current Audio block on master by:

  • always rendering BlockControls (that means both when state.editing is false or true)
  • making the button work as a toggle, with a switchToView callback, similar to switchToEditing
  • adding to the button a is-active CSS class and aria-pressed="true" when editing, like other buttons that have a "pressed" state

Quickly testing that, seems to me:

  • the focus loss doesn't happen
  • the button stays in its position, solving the problem triggered by the Fill/Slot thing
  • the UI better communicates its state

Would this help to solve the issue and improve consistency? /Cc @draganescu

Quick GIF from current master (with the old icon):

audio swap

@draganescu
Copy link
Contributor

@afercia I have implemented your idea in the six media blocks PRs and it worked like a charm. Will create a separate PR with the refactoring for the image block too. Great thinking! Thanks :)

@karmatosed
Copy link
Member

@draganescu as I see you made a few PRs, is this issue now able to be closed? If not, what is the next step forward?

@draganescu
Copy link
Contributor

Hey @karmatosed, here it goes:

  • the initial exploration was ok, so I started migrating it to other blocks. This issue seemed to be "close-able".
  • First I opened one PR per block.
  • Then closed them because we found that there was a better way to make one component to handle the editing flow in all media components.
  • Then @mtias argued that the icon didn't solve the problem, so we moved towards having a text button.
  • Then we stepped back from having one component that would wrap all media components to - having one edit button component.
  • Then @mtias and @youknowriad suggested we should not have it in other components but change one media component first.
  • Now I am 99.99% done with the whole thing in this PR Media flow component for media replace #16200 which only needs someone to approve it, and it was very hard for me to get that to happen. Design and interaction wise @mapk and @jasmussen are happy so I hope this lands in master asap.

Now if this new image replacement flow works out OK, I will implement it in all the other blocks and only then this issue is completed :)

@karmatosed
Copy link
Member

Ok that sounds great, it sort of sounds like we can close this issue is that right @draganescu?

@draganescu
Copy link
Contributor

I started to move the media replace flow to the other blocks, since the final form of it is merged and appears to work allright.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment