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

Use a light wrapper for the image block #20576

Merged
merged 7 commits into from Mar 5, 2020
Merged

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Mar 2, 2020

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 on data-* 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

  • Test the image alignment in different situations
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 2, 2020

Size Change: +140 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-editor/index.js 105 kB +3 B (0%)
build/block-editor/style-rtl.css 10.5 kB +36 B (0%)
build/block-editor/style.css 10.5 kB +37 B (0%)
build/block-library/index.js 116 kB +39 B (0%)
build/edit-post/style-rtl.css 8.54 kB +13 B (0%)
build/edit-post/style.css 8.54 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.36 kB 0 B
build/block-library/editor.css 7.36 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.9 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@ellatrix ellatrix force-pushed the try/light-wrapper-for-image branch from ea57f4e to f96118d Mar 4, 2020
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Mar 4, 2020

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Mar 4, 2020

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.

// 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 }>

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Would be cool if we could rewrite this as a hook. Something to explore separately.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 4, 2020

Author Contributor

Definitely, I actually did something like that a long time ago when I was playing with React hooks before their release #11161

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Awesome!

return (
<>
{ getInspectorControls(
imageWidth,
imageHeight
) }
<div style={ { width, height } }>
<ResizableBox

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Same here, I have a feeling that this could be rewritten as a hook in the future. :)

@ellatrix ellatrix force-pushed the try/light-wrapper-for-image branch from 3fdf2a6 to 3eb1b64 Mar 4, 2020
@youknowriad youknowriad requested review from nerrad and ntwb as code owners Mar 4, 2020
&[data-align="wide"] {
&[data-align="wide"],
&.alignfull,
&.alignwide {

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Hm, ideally this should be provided by the theme, no? I mean, ideally we shouldn't do any special handling for these classes.

@@ -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;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Why is this removed? This ensures that the movers are visible for "full" alignment.


&[data-align="full"] > .block-editor-block-list__insertion-point {
left: 0;
right: 0;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Mar 4, 2020

Member

Makes sense, this looks like a dead CSS rule to me.

Copy link
Member

ellatrix left a comment

Looks good. I just think this CSS rule should be restored: https://github.com/WordPress/gutenberg/pull/20576/files#r387703250

@ellatrix ellatrix force-pushed the try/light-wrapper-for-image branch from 7910228 to e7b0466 Mar 4, 2020
@youknowriad youknowriad merged commit 81d8afc into master Mar 5, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the try/light-wrapper-for-image branch Mar 5, 2020
@youknowriad youknowriad added this to the Gutenberg 7.7 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.