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

Image & Cover Block SCSS inconsistent with Media&Text & Embed Block - Forces Theme Acrobatics #11234

Closed
timhibberd opened this issue Oct 30, 2018 · 15 comments · Fixed by #17737
Closed
Assignees
Labels
Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress

Comments

@timhibberd
Copy link

Describe the bug
Image Block (.wp-block-image) & Cover Block (.wp-block-cover, .wp-block-cover-image) SCSS are setting the block width to 100% and the left / right margins to 0 in isolation from .alignfull / .alignwide. This forces the theme to go to unnecessary lengths to get the blocks to fit in containers that have margins due to the standard WordPress loading sequence of theme / plug-in css files since the block width and margins are given priority over the theme styles which are loaded earlier.

The Image Block & Cover Block inconsistently set max-width, width, and left / right margins relative to Media & Text Block and Embed Block. The latter two are a breeze for the theme to style. I'm not sure why the Image Block and Cover Block do not have SCSS configured the same as Media & Text Block and Embed Block w.r.t. Width, Max-Width, left / right margins.

Here is what it looks like in the theme...

Embed Block (Easy Fit into a container with margins):

image

Media & Text Block (Easy Fit into a container with margins):

image

Image Block (Not a natural fit in a container with margins...would need to use !important to override):

image

Cover Image Block (Not a natural fit in a container with margins...would need to use !important to override):

image

To Reproduce
Simply look in the SCSS file and the output CSS files. See screenshots above for clearer visual explanation.

Expected behavior
Left/right margins should only be set to 0 and the width set IF .alignwide or .alignfull is in play AND Gutenberg should consider carefully if it needs to style unqualified block classes at all. If all base block classes were always styled in the GB editor paired with a qualification class (never unqualified) then the theme can easily target any override if necessary because it always has targeting context. The unqualified base class is tough for the theme to target accurately in all cases without resorting to tricks or more complex overrides. For easier theme targeting GB could introduce a .noalignment class to complement the .alignwide and .alignfull. This provides a simple theme class targeting model because ALL the GB editor styling becomes contextually qualified which is easy to target in the theme. Yes...there are always CSS tricks to get the job done and, yes, we can register to override all the block styling...but it would be preferable not to when a small adjustment would make everything so much simpler.

For those of you shaking your heads going...no that won't work...I leave you with the following question...

The question to answer here is...if Media & Text Block and Embed Block don't need left / right 0 margin AND width on the unqualified block base class WHY does the Image Block & Cover Block need to. I can't see why they couldn't all be consistent. And, if I was to choose one of the two left / right 0 margin AND width styles for consistency...I would choose Media & Text Block and Embed Block style (i.e. no unqualified left / right 0 margin AND width).

So the following is hard for the theme to style without needing to register block override styling:

.wp-block-image { max-width: 100%; margin-bottom: 1em; margin-left: 0; margin-right: 0; }

The following is easy for the theme to style without needing to register block override styling:

.wp-block-image .alignfull { max-width: 100%; margin-bottom: 1em; margin-left: 0; margin-right: 0; }

Screenshots
See above

Desktop (please complete the following information):

  • OS: Windows
  • Browser Firefox
  • Version 63.0

Smartphone (please complete the following information):
Not tested

Additional context
Using core (5.0-beta2-43845. Gutenberg plug-in not active. This bug is a design issue not an operational issue. The issue applies within the current Gutenberg source code and, in-turn, the Core v5x source code.

Here, for example, is the Gutenberg Image Block SCSS showing that the 0 margin and 100% are NOT within the .alignfull context as I would suggest they should be. NOTE that the comment expresses qualification but the scss does not ( // This resets the intrinsic margin on the figure in non-floated, wide, and full-wide alignments. ):

image

CAVEAT: I defer to those who consider themselves CSS experts to wade in. Me...I'm happy to admit there are people who know CSS better than I. I think my suggestion is workable. I leave it to all you wiser CSS experts to judge...be kind :-)

@timnicholson
Copy link

I am a theme developer as well and came here to see what the plan was to make theme support very simple with the new GB editor. I am SUPER excited about GB and what that means for theme developers. I have spent a ton of time trying to write widgets and shortcodes to accomplish these types of things, but the user experience was terrible with trying to use them in the editor (even though they work great in widget areas).

I too would very much like consistent CSS applied in GB that easily allows themes to contain content, resize it for various screen sizes, including collapsing things that are in columns (generic column block and Media & Text). I'd also like to see consistent margin and paddings applied in GB that of course are easy to override in the theme.

I've been using the latest GB plugin along with the Gutenberg Starter theme, Atomic Block theme, and my own themes. I can tell you that I agree that it seems difficult to do the things above, such as get consistent max-width on the content blocks while having wide and full-width work properly and look good together. Controlling margins and padding consistently also seems hard.

There also seem to be big differences between what the editor shows and what the published page or post shows and certainly having them be exactly the same is a major goal of GB. Especially as it relates to widths and when colors are applied to blocks (and if the theme is set to have a background color it gets worse).

@timhibberd
Copy link
Author

@timnicholson - hopefully we can get some visibility by the core dev team before v5.x ships and it becomes messy to change (the QWERTY effect).

@danielbachhuber - I'm sure you have a million things on your plate but could you spare 5 minutes and have a look at this issue and tell us what you think? I believe a minor change now pre-v5x core release will help a lot of legacy themes comply easily. I'm not proposing a major CSS model overhaul...just a minor styling convention regarding GB Block base classes...let's call it "The principle of qualified styling". In a nutshell I am suggesting that GB introduce the .alignnone to complement the .alignfull and .alignwide and refrain from styling the block's base class. See issue details for reason why. Thanks in advance...much appreciated :-)

@timhibberd
Copy link
Author

NOTE: I have create a corresponding bug report in Trac since this issue should be evaluated and accepted or discarded BEFORE the release of WordPress V5.0. If this change is not made before the V5.0 WordPress release then the cost to unravel it after all the themes have created theme-specific barnacles may be too high and we will need to live with this inconsistency forever...or wear a lot of pain to get consistency back after the fact.

Just one man's two cents worth :-)

See https://core.trac.wordpress.org/ticket/45234#ticket.

@timhibberd
Copy link
Author

@bradyvercher - If it is not imposing, could you have a quick look at this issue I just filed? You raised a similar issue last December which was closed only on the condition that the issue you raised be revisited once we got closer to a final release. Well...we're coming down to the wire and I would like to see the same block alignment consistency you asked for last December. Thoughts? Thanks in advance...much appreciated :-)

@youknowriad
Copy link
Contributor

cc @kjellr @jasmussen Any thoughts here? Is this addressed?

@timhibberd
Copy link
Author

timhibberd commented Oct 2, 2019

Cheers @youknowriad for having a look. This is still the same issue as a year ago. I am not sure why margin-left / margin-right is set to 0% outside the context of .alignfull in the .wp-block-image scss file.

image

Interestingly...if you search the src folder for alignfull only the image-block has a reference and it has margin 0 outside the alignfull context.

So a related question is how is alignfull implemented now for all the other blocks (for example .wp-block-video doesn't reference alignfull or alignwide at all in the scss file) and why is .wp-block-image different?

As a theme designer I'm lost as to where .alignfull / .alignwide is now being taken into consideration in the block scss files.

Thanks for your help...much appreciated :-)

@kjellr
Copy link
Contributor

kjellr commented Oct 2, 2019

👋 Thanks for the ping — I hadn't seen this issue before. I did a little digging, those left/right margin resets were added in #10228 to fix an editor problem. I'm not sure they are actually necessary outside of the editor. .

I tried a handful of themes (Twenty Nineteen, Twenty Twenty, Twenty Seventeen, Twenty Sixteen, Gutenberg Starter Theme, etc.), and they all either override this rule or provide zero left/right margins for figure elements on the front end anyway. So it doesn't seem like it would be an issue to move this rule into editor.scss instead.

@jasmussen would you mind taking a look to confirm whenever you have a moment?

@timhibberd
Copy link
Author

timhibberd commented Oct 2, 2019

Does anyone know where .alignfull went in the gutenberg code? Other than this anomalous reference in wp-block-image and a second reference to alignfull in the feature wrapper in gutenberg/post-content.php (and some test /doc files) it seems to have evaporated from the Gutenberg code (I did a repository search).

I think before anyone considers changing / not changing this alignfull reference we need to figure out where alignfull went! As a theme designer .alignfull support is something I am expected to support but now it no longer seems to be referenced. How does a theme designer override the CSS to allow a block in a margined wrapper to be full width without .alignfull as an overrideable reference? Or is it now expected that a theme needs to wrapper gutenberg content in a zero left/right margined container at all times and leave the content margining to gutenberg?

Thanks in advance. Much appreciated.

@jasmussen
Copy link
Contributor

Does anyone know where .alignfull went in the gutenberg code? Other than this anomalous reference in wp-block-image and a second reference to alignfull in the feature wrapper in gutenberg/post-content.php (and some test /doc files) it seems to have evaporated from the Gutenberg code (I did a repository search).

I don't think there ever was specific .alignfull code output by Gutenberg itself. There are styles that target full-wide alignments for the editor specifically, but this usually targets [data-align="full"]. Like alignleft, I believe the idea is for theme developers to write their own alignfull rules in the theme, and that by opting in using add_theme_support( 'align-wide' );, they explicitly declare to have written those rules.

This is mainly because there are a million different ways to add wide alignments — negative margins, unbounded main column, vw units. And even then, what full-wide means can be interpreted differently by the theme, just like how TwentyNineteen interprets Align Right to pull it out of the main column.

For that reason, Kjell is right on the money, #10228 was the culprit.

As noted just a moment ago, the editor has CSS to handle images full-wide, but leaves it up to the theme to decide how to do that in the theme.

Therefore the fix I introduced to the margins should have been editor only.

I have created #17737 to fix that.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 3, 2019
@timhibberd
Copy link
Author

Cheers @jasmussen I appreciate the insight.

A year ago when this issue was raised .alignfull was THE css tag for full-width blocks. It looks like over the past year [data-align="full"] has been added to the code. I checked the latest twenty-nineteen theme and it does, indeed reference [data-align="full"].

It is tough for us legacy theme designers to keep up with the vast changes that have occured over the last year. I missed that change. Thanks for the clarification.

But now I am more confused. If you do a scan of the latest twenty-nineteen theme it uses both [data-align="full"] AND .alignfull in different contexts. Is [data-align="full"] used solely for the editor's rendering of the block's width and .alignfull used for the end-user rendering of the block's width? If so, why have two...is it that editor container wrappers are by nature different dimensions than the equivalent end-user render of the same blocks? It strikes me that moving towards a full WYSIWYG experience that the css tags for the editor and the end-user views would be the same shouldn't they...they were a year ago when this issue was raised (i.e .alignfull). Sorry to be inquisitive and thanks in advance for your patient answers. Much appreciated as always :-)

@jasmussen
Copy link
Contributor

Yes, I can understand the confusion. Let me help elaborate a bit.

[data-align="full"] has been part of the codebase from the start, but themers arguably should not be using it, and TwentyNineteen is arguably doing it wrong, though I'm sure they had a good reason to do it.

So why do we have both [data-align="full"] and .alignfull? Well, one is for the editor view — the data attribute — and the other is for the theme. So in that vein, nothing has changed. As a themer you should still be targetting .alignfull. The data attribute is for when you are styling the editor.

The logical next question then becomes: why do we need both? Why not just pick one?

Because for the foreseeable future (until we can use Shadow DOM and the CSS property display: contents;), the markup in the editor will be different from the markup in the rendered preview/theme in a few substantial ways. This is because in the editor you will always have to have additional UI. Take the Image block, for example, when selected it outputs a rich text field allowing you to add a caption. That rich text field has to be present in the DOM, obviously, and this makes the Image block have different markup than the frontend Image you receive on the end. Similarly, the image can be made full-wide in the editor — but for the UI surrounding the base Image block markup to also be aware that the Image block is full width, it needs that extra attribute.

In other words, as a themer, you should not need to know about the data attribute at all. Our goal is for styling the editor to be as easy as styling the theme itself. We're obviously not there yet, but this is the goal.

Hope that helps, and happy to answer any questions.

@timhibberd
Copy link
Author

Cheers @jasmussen much appreciated :-) So I assume that the twentynineteen developers were styling [data-align="full"] in the editor context to make it render as close as possible to the end-user render (styled .alignfull) so the end-user's got a reasonably "end-user" view while editing the page.

Have I got it right?

@jasmussen
Copy link
Contributor

Exactly correct.

This is, by the way, also something we would love to make as simple as possible. As you can see with TwentyNineteen, you can get REALLY close. The end result is a great user experience where the Preview button is almost not even needed, because what you see in the editor is the same as the published result, more or less. But as you can also see from looking at TwentyNineteen's style-editor.css, it's not trivial to get there. But it should be, and although we're busy doing a million things, we want to make it trivial to get there. For now, the documentation on editor styles is in https://developer.wordpress.org/block-editor/developers/themes/theme-support/#editor-styles.

@timhibberd
Copy link
Author

Cheers...you and the GB team are all doing fantastic work. It's tough for us part-time legacy theme developers / maintainers to keep up but the end result will be well worth any transitional head-scratching on our part. Consider me a big fan and supporter :-)

@jasmussen
Copy link
Contributor

Thanks so much, that really means a lot. There's lots of work to still be done, and it is absolutely a goal to make your life easier as a developer, in addition to easier for users, and we'll keep on it until we're there.

jasmussen added a commit that referenced this issue Oct 3, 2019
Fixes #11234. Alternative to #10228.

This PR fixes an issue where styles meant for the editor bled into the theme.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants