adds new edit flow for the cover block#15519
Conversation
draganescu
requested review from
Soean,
ajitbohra,
gziolo,
jorgefilipecosta,
noisysocks,
notnownikki,
talldan and
youknowriad
as code owners
kjellr
added
[Block] Cover
|
Design-wise, this is looking great on my end. 👍
|
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
Thanks for splitting these out into separate PRs, makes it so much easier to do a thorough review!
I spotted a few things that would be good to address.
gziolo
added
the
Good First Review
draganescu
force-pushed
the
update/new-edit-flow-cover
branch
from
cdc0d64 to
2d153a5
Compare
| } ); | ||
| } | ||
|
|
||
| this.toggleIsEditing(); |
There was a problem hiding this comment.
I feel like it might be safer if this could be called with an argument to force the value of isEditing - this.toggleIsEditing( false ). That would specifically turn off editing mode. That would prevent a scenario where onSelectUrl is called when isEditing is false resulting in a transition back to the placeholder.
| ), | ||
| } ); | ||
|
|
||
| this.toggleIsEditing(); |
There was a problem hiding this comment.
Same comment as above here. I think this should specifically set isEditing to false instead of just toggling.
| </> | ||
| <Toolbar> | ||
| <IconButton | ||
| className={ classnames( 'components-icon-button components-toolbar__control', { 'is-active': this.state.isEditing } ) } |
There was a problem hiding this comment.
Still seem to have components-icon-button class here, which can be removed.
|
When I go to swap the image out, I don't see the thumbnail of my current image in the placeholder screen. I'm using Firefox.
|
Ah, that's a good point. If we can add that in, that'd be great. |
There was a problem hiding this comment.
Hi @draganescu, cover supports video and images as backgrounds, and there is an attribute to differentiate between both, this PR does not changes that attribute, and we keep it equal to the previously used or we default to the image if the block was just created.
I think we should set this attribute in accordance with if the URL is an image or a video.
There are two ways to go:
- Add UI to allow the user to choose if an image or a video is being inserted.
- Automatically detect if the URL is a video or an image.
If we follow the UI path possible solutions are having two different buttons on that allows inserting an image and other that allows inserting a video. Or, we can have one button but add a select control at the side of the URL.
If we follow the automatic approach we have two options:
- Make a headers request to the URL and verify its mime type, for URL in other domains we may have cross-domain request problems.
- Add the URL invisible elements containing image and video, then use the DOM API to understand if the elements include an image or video I guess we can guess using the dimensions of the element and other properties that should only be available if the element loaded with success. This approach is hacky but is probably very reliable, I guess there may be some npm package that does this, and if not it may be interesting to create a simple one.
|
|
||
| const onSelectUrl = ( newURL ) => { | ||
| if ( newURL !== url ) { | ||
| this.props.setAttributes( { |
There was a problem hiding this comment.
We are not setting the media type so, If I have a video as the cover, and I switch to use an image by URL things fail because the media type is still video.
draganescu
force-pushed
the
update/new-edit-flow-cover
branch
from
2d153a5 to
017179e
Compare
|
#11952 changed direction so closing this as it became irrelevant. |
7 participants
Description
Closes #14795, #10853, #16350
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