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

adds new edit flow for the embed block #15518

Closed
wants to merge 4 commits into from

Conversation

draganescu
Copy link
Contributor

Description

Closes #14795

This is a follow up to the image block edit flow update which ports the new flow to all the blocks which have media with an edit state.

How has this been tested?

For now I've only tested locally.

Types of change

New feature (non-breaking change which adds functionality)

Screenshots

embed

@kjellr
Copy link
Contributor

kjellr commented May 9, 2019

When testing this on the Youtube block, I'm seeing the Cancel link show up after pasting in an embed URL for a freshly-created block. When this happens, the button isn't clickable. We should hide the cancel link entirely unless you're in the "Edit URL" mode:

youtube

@kjellr kjellr added [Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement. labels May 9, 2019
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label May 10, 2019
@draganescu
Copy link
Contributor Author

hi @kjellr I have fixed that wrong behaviour and also rebased the PR.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @draganescu. Tested with a handful of different embeds and this seems to work well.

I did find a weird bug though: sometimes it appears that the toolbar position of the swap icon jumps around after the embed has been changed. At first, it appears to the right of the alignment controls, but after a swap it sometimes shows up on the left instead:

embed-2

To reproduce:

  1. Manually create a new embed block (I've reproduced this with the Youtube and Soundcloud blocks)
  2. Add a URL to start with, hit "Embed"
  3. Press the replace icon.
  4. Add a different embed URL, hit the "Embed" button again.
  5. See the new position for the icon.

This isn't happening 100% of the time for me, but I'm seeing it about 80% of the time... so those steps may not be totally exact. Let me know if you can't reproduce and I'll do some more digging too.

@mapk
Copy link
Contributor

mapk commented May 28, 2019

I caught that bug myself, @kjellr and noted it on #15516 (comment) before seeing your report here.

@draganescu
Copy link
Contributor Author

@mapk , @kjellr that bug is happening because the new button uses component state to decide if it is rendered as pressed or not. Because of this the block controls are re-rendered and this ruins the order of the items in the toolbar.

I have discussed this with @youknowriad and it is a higher order issue in the Fill/Slot component. I confirmed it affects at least on other block, Audio, on current master.

Fixing this requires further investigation and it is beyond the scope of these PRs.

@afercia
Copy link
Contributor

afercia commented Jun 7, 2019

Related to the "swapping" (and sometimes disappearing) icon: #16048

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

The editing flow I tried here doesn't quite work for me.

  1. Embed a url
  2. Click the new Edit icon
  3. Change the URL
  4. Click the Edit icon again to go back to the preview

The URL doesn't update, the embed is the old URL.

  1. Click the Edit icon again
  2. Click cancel

The preview of the original URL returns.

  1. Click the Edit icon again

The edited URL is still there - the edit has not been canceled.

Also, I wouldn't have guessed the icon was "edit" - it looks just like the icon we use to change block types. @jasmussen could we get your feedback there? Is this something that has changed all over the place and we're expecting users to "just know"? The only way I knew it was edit was to hover over the icon and wait for the hint to appear, and as we've already got the association in Gutenberg of cycling arrows means changing block type it seems confusing and I'd never have tried if I hadn't seen this PR.

Screenshot from 2019-06-10 11-44-01

edit

@jasmussen
Copy link
Contributor

Also, I wouldn't have guessed the icon was "edit" - it looks just like the icon we use to change block types. @jasmussen could we get your feedback there?

I would go for the plain edit icon.

The flow to revert the block back to its setup state does feel like it could use some high level thought, same as for the Image block, but given that's how it works here right now it's probably fine to move forward since it solves a problem. Can always revisit once a better flow pattern is established in another block.

@kjellr
Copy link
Contributor

kjellr commented Jun 10, 2019

I wouldn't have guessed the icon was "edit" - it looks just like the icon we use to change block types. @jasmussen could we get your feedback there? Is this something that has changed all over the place and we're expecting users to "just know"? The only way I knew it was edit was to hover over the icon and wait for the hint to appear, and as we've already got the association in Gutenberg of cycling arrows means changing block type it seems confusing and I'd never have tried if I hadn't seen this PR.

Interesting — I hadn't considered the confusion with the block type switcher. This is something we're changing in a lot of different places right now, as per #14795.

In the case of the image block, I think that icon makes a lot of sense, since you're more clearly "swapping" one image another. When the item you're changing is just a URL though, perhaps a more generic "edit" icon would make more sense. 🤔 Curious if any other designers have thoughts on this one.

@draganescu
Copy link
Contributor Author

#11952 changed direction so closing this as it became irrelevant.

@draganescu draganescu closed this Jul 31, 2019
@youknowriad youknowriad deleted the update/new-edit-flow-embed branch May 27, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand the new replace image flow out to other blocks
7 participants