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

Fix fullwide regression. #21201

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Fix fullwide regression. #21201

merged 2 commits into from
Mar 30, 2020

Conversation

jasmussen
Copy link
Contributor

Full-wide blocks were no longer full-width. This PR fixes that.

This possibly regressed in #21099.

Before:

Screenshot 2020-03-27 at 15 11 36

After:

Screenshot 2020-03-27 at 15 16 50

@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Mar 27, 2020
@jasmussen jasmussen self-assigned this Mar 27, 2020
// Full-wide needs negative margins to accommodate the root container padding.
&[data-align="full"],
&.alignfull {
margin-left: -$block-padding;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should retire/rename both of the variables used here, as they are no longer descriptive of their original intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kept this style but I moved it to the place where we add the padding to the block wrapper.

Do we really need to add it here? Why?

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list/style.scss#L650-L662

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know... Do you not see the margin issue in master? Is it just a cache ghost ok my machine? Note that I'm using a theme that doesn't provide an editor style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it seems to happen only for themes without editor styles, still trying to figure out why

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I figured it out, it depends on the block. Try Media&Text for instance, you'll notice that it works just fine.

Copy link
Member

Choose a reason for hiding this comment

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

I need to run some errands, but I'll have a look after that in a bit. I think somehow we need alignfull/wide on the wrapper div.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the problem... It seems fines even without editor styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually scratch what I said about editor styles. This is also in TwentyTwenty:

Screenshot 2020-03-28 at 13 46 14

It's because of this:

Screenshot 2020-03-28 at 13 45 29

That div wraps the Image block, which means it isn't targetted by this:

.block-editor-block-list__layout.is-root-container > .block-editor-block-list__block[data-align="full"]

I think we need to do two things:

  1. Figure out why the image block has an extra div in the editor — was a new fragment introduced?
  2. Relax or reconsider the idea of scoping the full-wide rule.

What do you think, @youknowriad?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I haad the same thoughts here

#21201 (comment)

I think we should relax the rule for now and if possible try to find a way to not apply it in inner blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree, but your point about the group block was well made. And if we change the current code (https://github.com/WordPress/gutenberg/pull/21201/files#diff-ee2ed3adbe2578628039530c717a9a93R652) to just relax it overall, that regresses the column block with wide blocks inside.

@jasmussen
Copy link
Contributor Author

This one would be good to get in along with #21097 ;)

@github-actions
Copy link

github-actions bot commented Mar 27, 2020

Size Change: +58 B (0%)

Total Size: 857 kB

Filename Size Change
build/block-editor/style-rtl.css 11 kB +26 B (0%)
build/block-editor/style.css 11 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.45 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-editor/index.js 102 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.5 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.5 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.25 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 91 kB 0 B
build/edit-post/style-rtl.css 8.43 kB 0 B
build/edit-post/style.css 8.43 kB 0 B
build/edit-site/index.js 6.73 kB 0 B
build/edit-site/style-rtl.css 2.91 kB 0 B
build/edit-site/style.css 2.9 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.57 kB 0 B
build/edit-widgets/style.css 2.57 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.38 kB 0 B
build/editor/style.css 3.38 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 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.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 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.7 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.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 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.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 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

@jasmussen jasmussen mentioned this pull request Mar 27, 2020
6 tasks
@jasmussen
Copy link
Contributor Author

I don't think this is world ending and in need of an urgent patch. If it only happens if you use a theme that doesn't style the editor but does support full width, it's already edge case. I think it's okay if this needs more time in the oven.

@jasmussen jasmussen force-pushed the try/full-wide-regression-fix branch 2 times, most recently from 0d11df6 to 9a47aaa Compare March 28, 2020 13:18
@jasmussen
Copy link
Contributor Author

Alright, I have created a different fix based on the learning in #21201 (comment).

The Image block has an alignment wrapper that outputs a div around the image block with a specific class, if necessary. It looks like this:

				<div
					className={
						// Ideally these classes are not needed, and ideally, we
						// provide an alignment wrapper component that the block
						// can wrap around the block or we build it into
						// Block.*.
						needsAlignmentWrapper
							? 'wp-block block-editor-block-list__block'
							: undefined
					}
				>

When the image block has no alignment, or it has full-wide, then that div is just an empty wrapper.

In leiu of refactoring that div away, I created a fix that targets full-wide children of empty divs as well. It's not the prettiest solution in the world, but it's a very simple fix that would be good to ship as we look for better solutions. What do you think, @ellatrix?

@jasmussen jasmussen force-pushed the try/full-wide-regression-fix branch from 9a47aaa to 1b421e4 Compare March 30, 2020 06:29
@@ -649,6 +649,9 @@

.block-editor-block-list__layout.is-root-container {
// Full-wide. (to account for the padddings added above)
// The first two rules account for the alignment wrapper div for the image block.
> div > .block-editor-block-list__block[data-align="full"],
> div > .block-editor-block-list__block.alignfull,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add > div:not(.block-editor-block-list__block) instead of > div because that markup is definitely possible in nested contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we're already in "very very specific selectors" territory — but the :not rule would make it even more specific. But I'm happy to add it.

@jasmussen jasmussen merged commit 30536e6 into master Mar 30, 2020
@jasmussen jasmussen deleted the try/full-wide-regression-fix branch March 30, 2020 09:43
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Mar 30, 2020
@ellatrix
Copy link
Member

Maybe I should have included this in the patch release 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants