Update/edit flow for blocks which have a media placeholder #14918

draganescu · 2019-04-10T21:29:31Z

Description

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. The blocks affected are:

media+text
cover
audio
video
file

How has this been tested?

For now I've only tested locally.

Types of change

New feature (non-breaking change which adds functionality)

Screenshots

Media + text

Cover

Audio

File

Video

Embed

packages/block-library/src/video/edit.js

kjellr · 2019-04-11T16:11:55Z

This is looking great so far, @draganescu! I took it for a quick spin, and noticed just a couple small bugs:

Would it be possible to add a "Cancel" button to the embeds?

It looks like the image preview is a little misplaced in the Media + Text block. I think we need to adjust the margins + max-width there to be a bit more comfortable:

draganescu · 2019-04-11T17:08:27Z

@kjellr so far there are three problems I know of:

~~the file block can't work with an URL~~ the file block does work with URL!
the embed block doesn't have a cancel link (like you've also noticed)
the image in the media+text block (you found that :D I didn't see it b/c I used a square image)

Also I have marked this WIP b/c some of the code could be made more DRY and the edit/view pattern could be made the same in all blocks, maybe have an edit/view wrapper component?

draganescu · 2019-04-15T13:46:06Z

Rebased and updated with new snapshot so tests will pass. I think it's ready for review. Aside from code DRY-ness, there might be a point where we could make the code in these blocks more "standard" but maybe we could move ahead like this for now and refactor in a separate PR.

@kjellr if you can take it for another spin that'd be great! :)

kjellr · 2019-04-18T16:26:18Z

Thanks, @draganescu! This is looking great. I noticed just one small bug:

For embed blocks, the "Cancel" link appears even before you enter a URL:

Once that's all set, I think this is all set from a design perspective. Before we decide to move ahead for sure though, I'd love to get @jasmussen's thoughts on the DRY issue. He may have an idea for a quick fix.

jasmussen · 2019-04-19T06:32:08Z

This is nice work, and it solves a problem, which is great. Like Kjell, I think it's close to shipping as an enhancement to what we have. There's just one issue that needs to be fixed before that:

In this screenshot the edit image button appears to be part of the alignment group. The edit image button should always be its own group, and never be part of any other groups, because its behavior is different.

All this said, I feel like this interface also starts to show some of the limitations of the current placeholder interface we have, and suggests that we should separately look at improving this in a holistic way.

We have a generic appender for adding items, which features a dashed outline but same gray.
There's no dashed outline on the media placeholder, but there's a whole slew of buttons that are increasingly unwieldy and missing and some layout.
The flow for going from final state and back to setup state is a little inconsistent.
The button for going back to this state implies "Switch out", which is true, but it's still a little obtuse. And moreso, when it sits in the block toolbar it feels like it implies action on the whole block, which in the case of the Media & Text block is not the case. I wonder if it might be more clear to add the "Edit image" button to the overflow menu instead?
Sometimes clicking the edit button shows a preview of what's currently applied (image), other times it doesn't (embeds).

I don't feel like those issues are blockers for shipping this improvement. But it does feel like there is an opportunity to step back for a second and think: if we tackled appenders, edit buttons, and placeholder/setup states together, what could that do to improve the user experience? What could we eliminate? What could we improve?

draganescu · 2019-04-19T10:11:38Z

rebased, fixed the button'd group (thanks @jasmussen), removed the cancel link (thanks @kjellr).
waiting on someone to review the code :)

@afercia if you could take it for a spin to make sure I didn't introduce accessibility issues with this update, that'd be awesome, thank you!

talldan · 2019-05-02T09:56:16Z

This one might be easier to review (and ship) if it were one PR per block.

I'll post some observations from testing as a comment and then loop back to a code review:

Media & Text

Some space is needed between the Media Library and Insert From URL buttons (seems to also apply to other blocks). This does seem to be an issue with the image block as well, so could always fix it after this PR for all blocks:
I have access to the Insert from URL option when logged in as a contributor, but the image is not added when I use it (it appears blank):

Cover

As a contributor, the edit image toolbar button doesn't show up after inserting from a URL (seems to work nicely as an admin, though):

Audio

The media library seems to be able to get a 'poster' for an audio clip. A future improvement might be to see if we can also access this and use it as the preview on the placeholder. The same may also be true for videos:

File

I'm seeing an issue using the Upload button from the placeholder. The upload seems to take place but the block immediately shows the placeholder again.
The filename and file link isn't updated when adding or replacing a file using 'Insert from URL' (could just use the url).
As a contributor, there's no cancel button when replacing a file. (seems to be an issue for multiple blocks)

talldan

I've made my way through about half the code, but there's quite a lot. I defo feel this could be split up into a PR per block. It'd be much easier to refine and ship each one indivdually.

One thing that having all the changes in one PR makes clear is how different yet the same all the media based blocks are. It'd be great to see some steps towards making the code the same across each one, even if it's just using the same style of code (methods vs inline functions, naming conventions).

Then it'd be easier at a later point to try to refactor them all to use some common code.

talldan · 2019-05-02T09:59:13Z

packages/block-library/src/audio/edit.js

@@ -103,7 +107,7 @@ class AudioEdit extends Component {
 		const { setAttributes, isSelected, className, noticeOperations, noticeUI } = this.props;
 		const { editing } = this.state;
 		const switchToEditing = () => {


The function should probably be renamed to something like toggleEditing.

I also noticed this function doesn't speak when switching while some of the other blocks do.

talldan · 2019-05-02T10:01:26Z

packages/block-library/src/audio/edit.js

+					<BlockControls>
+						{ !! src && ( <Toolbar>
+							<IconButton
+								className={ classnames( 'components-icon-button components-toolbar__control', { 'is-active': this.state.editing } ) }


No need to add the components-icon-button classname, this is added by the IconButton component itself.

Also, it seems like is-active would always be true as this is within an if statement if ( editing ) {

talldan · 2019-05-02T10:05:47Z

packages/block-library/src/audio/edit.js

@@ -143,7 +160,7 @@ class AudioEdit extends Component {
 							className="components-icon-button components-toolbar__control"


Same issue with the components-icon-button classname. Looks like it was an existing issue.

Seems a shame to have to duplicate the code for this IconButton. Maybe it can be extracted to a separate component in the same file? Or the rendering logic could be reworked.

talldan · 2019-05-02T10:22:44Z

packages/block-library/src/cover/edit.js

+						onDoubleClick={ this.toggleIsEditing }
+						onCancel={ !! url && this.toggleIsEditing }
+						notices={ noticeUI }
+						onError={ this.onUploadError }


Is this right? I couldn't see an onUploadError method.

talldan · 2019-05-02T10:25:04Z

packages/block-library/src/file/edit.js

+					<BlockControls>
+						<Toolbar>
+							{ !! href && ( <IconButton
+								className={ classnames( 'components-icon-button components-toolbar__control', { 'is-active': this.state.isEditing } ) }


components-icon-button again

talldan · 2019-05-02T10:26:20Z

packages/block-library/src/file/edit.js


-		if ( ! href || hasError ) {
+		if ( ! href || isEditing || hasError ) {


Might be able to drop ! href here, since isEditing is now initialised to the right value.

draganescu · 2019-05-02T13:51:31Z

Thanks @talldan! It feels like a great moment to refactor some code indeed, my fear is that other PRs on these blocks plus refactoring will make some mega conflicts given time :D. I will try to split in more than one PR and see how that goes.

Not sure about the issues when logged in as a contributor and how the media placeholder could have caused them :/

…d video blocks

talldan · 2019-05-07T02:50:49Z

@draganescu - we can try to push this through for all the blocks, but based on experience I've seen that smaller PRs tend to ship more quickly and are more likely to be reviewed. They're also much easier to test.

Focussing on one block at a time is what I'd personally try to do.

A good way to do it might be to checkout the files from this branch for one block onto a separate branch. Haven't tried it but something like this might work to bring the changes across onto a new branch:

> git checkout master
> git pull
> git checkout -b update/edit-flow-for-audio-block
> git checkout update/edit-flow-for-media -- packages/block-library/src/audio/

draganescu · 2019-05-08T19:52:09Z

@talldan as suggested I've split this into multiple PRs for easier review. I have also incorporated the bits collected from your past reviews (hopefully) in the new PRs.

Here they are:

Audio Block

#15520

Cover Block

#15519

Embed block

#15518

Media + Text Block

#15517

Video block

#15516

File block

#15515

draganescu requested review from ajitbohra, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners April 10, 2019 21:29

gziolo reviewed Apr 10, 2019

View reviewed changes

packages/block-library/src/video/edit.js Show resolved Hide resolved

draganescu mentioned this pull request Apr 10, 2019

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

Closed

6 tasks

talldan assigned draganescu Apr 11, 2019

talldan added the [Status] In Progress Tracking issues with work in progress label Apr 11, 2019

draganescu added Needs Design Feedback Needs general design feedback. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Apr 11, 2019

draganescu force-pushed the update/edit-flow-for-media branch from 467993b to 74c28dd Compare April 12, 2019 12:43

draganescu requested a review from chrisvanpatten as a code owner April 13, 2019 09:57

draganescu force-pushed the update/edit-flow-for-media branch from df469d8 to 8080368 Compare April 15, 2019 12:47

draganescu changed the title ~~WIP: Update/edit flow for blocks which have a media placeholder~~ Update/edit flow for blocks which have a media placeholder Apr 15, 2019

draganescu force-pushed the update/edit-flow-for-media branch from 8080368 to 72d6ba2 Compare April 19, 2019 09:25

draganescu added Needs Technical Feedback Needs testing from a developer perspective. and removed Needs Design Feedback Needs general design feedback. labels Apr 19, 2019

draganescu force-pushed the update/edit-flow-for-media branch from 36a6379 to dfbc4cd Compare April 22, 2019 15:09

draganescu force-pushed the update/edit-flow-for-media branch from 4b4b9a8 to 131fbcc Compare April 30, 2019 14:29

draganescu requested a review from aduth as a code owner April 30, 2019 14:29

draganescu mentioned this pull request May 1, 2019

Update: Image Block: Change image by drag & drop an image from the desktop #14983

Merged

talldan removed the [Status] In Progress Tracking issues with work in progress label May 2, 2019

talldan reviewed May 2, 2019

View reviewed changes

draganescu and others added 15 commits May 3, 2019 12:06

adds the new edit media icon to the media-text, cover, audio, file an…

13146a7

…d video blocks

add new placeholder to edit state of media text

c7c606e

fixes preview for media other than image

5646a72

adds the new flow to the cover block

cab27f9

adds correct state to edit button, adds new flow to audio

e3c3866

fixed state for newly inserted audio, adds new flow to file

a659801

adds aria pressed to edit button, adds the new flow to video

e9cd606

adds the new edit flow to embed

2723aa8

removed duplicate definition

0e7129f

adds cancel link to embed edit mode

18fdced

fixes the media preview in media + text block

d7c3b39

new snapshot for the embed block

5e23de5

removes the cancel link in embed initial state

84efe80

moves the edit button to its own group, fixes media preview margin

7948ece

new snapshot for the embed block

2b129f3

draganescu force-pushed the update/edit-flow-for-media branch from 131fbcc to 2b129f3 Compare May 3, 2019 09:07

talldan mentioned this pull request May 7, 2019

Common Block Functionality #15450

Closed

draganescu closed this May 8, 2019

youknowriad deleted the update/edit-flow-for-media branch May 27, 2020 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/edit flow for blocks which have a media placeholder #14918

Update/edit flow for blocks which have a media placeholder #14918

draganescu commented Apr 10, 2019 •

edited

Loading

kjellr commented Apr 11, 2019

draganescu commented Apr 11, 2019 •

edited

Loading

draganescu commented Apr 15, 2019

kjellr commented Apr 18, 2019

jasmussen commented Apr 19, 2019

draganescu commented Apr 19, 2019 •

edited

Loading

talldan commented May 2, 2019 •

edited

Loading

talldan left a comment

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

talldan May 2, 2019

draganescu commented May 2, 2019

talldan commented May 7, 2019

draganescu commented May 8, 2019

		@@ -143,7 +160,7 @@ class AudioEdit extends Component {
		className="components-icon-button components-toolbar__control"


		if ( ! href \|\| hasError ) {
		if ( ! href \|\| isEditing \|\| hasError ) {

Update/edit flow for blocks which have a media placeholder #14918

Update/edit flow for blocks which have a media placeholder #14918

Conversation

draganescu commented Apr 10, 2019 • edited Loading

Description

How has this been tested?

Types of change

Screenshots

Media + text

Cover

Audio

File

Video

Embed

kjellr commented Apr 11, 2019

draganescu commented Apr 11, 2019 • edited Loading

draganescu commented Apr 15, 2019

kjellr commented Apr 18, 2019

jasmussen commented Apr 19, 2019

draganescu commented Apr 19, 2019 • edited Loading

talldan commented May 2, 2019 • edited Loading

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draganescu commented May 2, 2019

talldan commented May 7, 2019

draganescu commented May 8, 2019

Audio Block

Cover Block

Embed block

Media + Text Block

Video block

File block

draganescu commented Apr 10, 2019 •

edited

Loading

draganescu commented Apr 11, 2019 •

edited

Loading

draganescu commented Apr 19, 2019 •

edited

Loading

talldan commented May 2, 2019 •

edited

Loading