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: unset max-width for reusable blocks #22632

Closed
wants to merge 1 commit into from

Conversation

dsifford
Copy link
Contributor

Description

This PR addresses the issue described in #8288.

See: #8288 (comment)

There are also other interesting ideas as to how we can improve this fix in the future, which can be addressed in a future PR.

If you all think this css should be more closely coupled with the reusable block (say in packages/block-library/src/block/editor.scss, for example) let me know and I'll move it there.

How has this been tested?

Unable to get wp-env running on my linux machine, so this was not tested here. But I did test this in another installation by applying a similar change to the css.

Screenshots

(from the issue thread)...

example

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

Size Change: +23 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/edit-post/style-rtl.css 12.2 kB +11 B (0%)
build/edit-post/style.css 12.2 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.48 kB 0 B
build/block-directory/style-rtl.css 788 B 0 B
build/block-directory/style.css 788 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.2 kB 0 B
build/block-library/editor.css 7.2 kB 0 B
build/block-library/index.js 119 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-site/index.js 14 kB 0 B
build/edit-site/style-rtl.css 5.52 kB 0 B
build/edit-site/style.css 5.53 kB 0 B
build/edit-widgets/index.js 8.05 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 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 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 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 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 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.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 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.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

Fast work, thank you for exploring in a PR!

When a theme does not supply an editor style, what you intended is what I see:

Screenshot 2020-05-27 at 10 26 57

On its own, I don't think this is terrible as it does fix the issue of allowing full-wide images inside.

However this PR also highlights an issue we have not thought about — editor styles. This is what I see in the theme I'm working on:

Screenshot 2020-05-27 at 10 24 32

That is — it's not full-wide, because my theme overrides that:

body > *:not([data-align="wide"]):not([data-align="full"]) {
    max-width: 960px;
    margin-left: auto;
    margin-right: auto;
}

the rules you wrote don't have enough specificity:

Screenshot 2020-05-27 at 10 26 20

It would be easy to increase that specificity to force it to work. But that would actually break my theme's editor style. Which is okay if we're sure about it, but it does raise the question: are we sure about it?

It touches again on #20650 specifically (CC: @youknowriad and @kjellr ) — how should a full-wide group block treat child blocks? Should child blocks be in a thin column just like they are in the root container? Or should child blocks be allowed to go full-wide because a group is just a thin wrapper?

The thing that makes this extra complex is that reusable blocks are only surfaced in the editor. Which means if they inherit editor styles, they will likely cause inconsistencies. So we arguably need a way to treat reusable blocks in a way that would be consistent with how an editor style might treat it. Which is difficult — because on the front-end/published side, the reusable block is not in a wrapper, it's transparent. We effectively need for the reusable block wrapper to be as invisible and transparent in the editor as possible. Something like display: contents;.

Adding a design feedback label to gather more thoughts, and thanks again for the PR, it's a great way to force the conversation!

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label May 27, 2020
@kjellr
Copy link
Contributor

kjellr commented May 27, 2020

It would be easy to increase that specificity to force it to work. But that would actually break my theme's editor style. Which is okay if we're sure about it, but it does raise the question: are we sure about it?

I'm not sure about it! 😄

I'd love to solve this in a way that avoids breakage — ideally themes wouldn't have to make any change to make this work right.

One thought is that we could we do something like we do for the Group block? For that block, when you "group" a full-wide block, it makes the Group full-wide by default. That would mean we'd need to add alignment controls on the reusable block, which is weird... and it would likely still require some level of editor style updates for themes. 🤔 On second thought, probably not a great idea.

It touches again on #20650 specifically (CC: @youknowriad and @kjellr ) — how should a full-wide group block treat child blocks? Should child blocks be in a thin column just like they are in the root container? Or should child blocks be allowed to go full-wide because a group is just a thin wrapper?

In normal cases, I'm still a solid no: Full-wide blocks should not go full-width if they're inside of a smaller container. I think of parent blocks like physical boxes: If you put something huge in a box, it's going to have to shrink down to fit.

But I think the case of reusable blocks, it's a little different — the reusable wrapper is meant to be truly invisible. So I think we could treat that a little differently if we needed to.

@jasmussen
Copy link
Contributor

ideally themes wouldn't have to make any change to make this work right.

This seems to be key. Reusable blocks should ideally be a transparent editor-only affordance.

One thought is that we could we do something like we do for the Group block? For that block, when you "group" a full-wide block, it makes the Group full-wide by default. That would mean we'd need to add alignment controls on the reusable block, which is weird... and it would likely still require some level of editor style updates for themes. 🤔 On second thought, probably not a great idea.

Wouldn't that require a theme to be aware of it? The thing is — you can make a single paragraph, ungrouped, into a reusable block. This wraps it in a div, and it's that div which causes all the trouble. If all reusable blocks were by definition turned into groups first, then we could potentially level the playing field by targetting the group styles in a generic way. But alas, I can think of a few good use cases for not grouping content just by making it reusable.

In normal cases, I'm still a solid no: Full-wide blocks should not go full-width if they're inside of a smaller container. I think of parent blocks like physical boxes: If you put something huge in a box, it's going to have to shrink down to fit.

Just to be sure I explained myself: if the group is fullwide, what should happen to a not-full-wide block inside? Should it expand to fit the available space (Figma behavior) or should it act just like it does in the root container, and be in a centered column? I know we've had this discussion before, and I don't necessarily mean to up-end it — strong theme folks like yourself should have the say here. I'm bringing it up because I believe #20650 means to take the vinegar out of wide, fullwide and normal blocks by handling it, so that we could apply a class to any container and make it behave like the is-root-container. If we did that, perhaps we simply apply that class also to reusable blocks?

@kjellr
Copy link
Contributor

kjellr commented Jun 3, 2020

Wouldn't that require a theme to be aware of it?

Yeah, I don't think my thoughts there make sense. 😄

Just to be sure I explained myself: if the group is fullwide, what should happen to a not-full-wide block inside? Should it expand to fit the available space (Figma behavior) or should it act just like it does in the root container, and be in a centered column?

I still think the Figma-like behavior is what's expected. Unless the container is meant to be 100% invisible to the user, like the special case we're talking about here.

@mapk
Copy link
Contributor

mapk commented Jul 30, 2020

One thought is that we could we do something like we do for the Group block? For that block, when you "group" a full-wide block, it makes the Group full-wide by default. That would mean we'd need to add alignment controls on the reusable block, which is weird... and it would likely still require some level of editor style updates for themes. 🤔 On second thought, probably not a great idea.

Can this be done programmatically? So if the inner block has a full-wide setting, the reusable block inherits that automatically.

Base automatically changed from master to trunk March 1, 2021 15:43
@skorasaurus skorasaurus added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Dec 8, 2021
@dsifford dsifford closed this Jun 9, 2023
@dsifford dsifford deleted the fix/reusable-block-max-width branch June 9, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants