Use a light wrapper for the image block #20576
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
requested review from
ajitbohra,
ellatrix,
jorgefilipecosta,
Soean and
talldan
as code owners
youknowriad
added
[Block] Image
|
Size Change: +140 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
ellatrix
reviewed
Mar 2, 2020
ellatrix
force-pushed
the
try/light-wrapper-for-image
branch
from
ea57f4e
to
f96118d
Compare
|
I rebased this PR (force push). |
|
One small thing I notice is that the movers can't be accessed if the image is full width. That's probably because the data- attributes are no longer there and the position of the toolbar won't be adjusted. |
ellatrix
reviewed
Mar 4, 2020
| // Disable reason: Each block can be selected by clicking on it | ||
| /* eslint-disable jsx-a11y/click-events-have-key-events */ | ||
| return ( | ||
| <> | ||
| { controls } | ||
| <figure className={ classes }> | ||
| <ImageSize src={ url } dirtynessTrigger={ align }> |
There was a problem hiding this comment.
Would be cool if we could rewrite this as a hook. Something to explore separately.
There was a problem hiding this comment.
Definitely, I actually did something like that a long time ago when I was playing with React hooks before their release #11161
ellatrix
reviewed
Mar 4, 2020
| return ( | ||
| <> | ||
| { getInspectorControls( | ||
| imageWidth, | ||
| imageHeight | ||
| ) } | ||
| <div style={ { width, height } }> | ||
| <ResizableBox |
There was a problem hiding this comment.
Same here, I have a feeling that this could be rewritten as a hook in the future. :)
ellatrix
force-pushed
the
try/light-wrapper-for-image
branch
from
3fdf2a6
to
3eb1b64
Compare
ellatrix
reviewed
Mar 4, 2020
| &[data-align="wide"] { | ||
| &[data-align="wide"], | ||
| &.alignfull, | ||
| &.alignwide { |
There was a problem hiding this comment.
Hm, ideally this should be provided by the theme, no? I mean, ideally we shouldn't do any special handling for these classes.
ellatrix
reviewed
Mar 4, 2020
| @@ -554,10 +556,6 @@ | |||
| margin-left: - $block-toolbar-height - $border-width; | |||
| } | |||
|
|
|||
| .block-editor-block-contextual-toolbar[data-align="full"], | |||
| .block-editor-block-list__breadcrumb[data-align="full"] { | |||
| margin-left: 0; | |||
There was a problem hiding this comment.
Why is this removed? This ensures that the movers are visible for "full" alignment.
ellatrix
reviewed
Mar 4, 2020
|
|
||
| &[data-align="full"] > .block-editor-block-list__insertion-point { | ||
| left: 0; | ||
| right: 0; |
There was a problem hiding this comment.
Makes sense, this looks like a dead CSS rule to me.
ellatrix
approved these changes
Mar 4, 2020
There was a problem hiding this comment.
Looks good. I just think this CSS rule should be restored: https://github.com/WordPress/gutenberg/pull/20576/files#r387703250
Co-Authored-By: Zebulan Stanphill <zebulanstanphill@protonmail.com>
ellatrix
force-pushed
the
try/light-wrapper-for-image
branch
from
7910228
to
e7b0466
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The biggest challenge here is alignments, and the approach I took is to try to match the frontend as closely as possible. In the frontend, for left, right and centered images, there's an extra "div" that is added as a wrapper.
The frontend also relies on
align*classNames to style the alignments while the backend used to rely ondata-*attributes. This consolidates on the classNames.So this PR is most likely to set a precedent to be followed in terms of alignment and something that might extend to all the other blocks.
Note
The focus style when the image is floated is kind of broken but it's not specific to this PR.
Testing