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.

Sign up for GitHub

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

Try moving block margins to each block #8350

Closed
wants to merge 2 commits into from
Closed

Try moving block margins to each block #8350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0137e8
Try moving block margins to each block
jasmussen Aug 1, 2018
819e251
Try a more blanket statement to target all blocks.
Aug 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions edit-post/assets/stylesheets/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ $block-parent-side-ui-clearance: $parent-block-padding - $block-padding; // spac

$block-container-side-padding: $block-side-ui-width + $block-padding + 2 * $block-side-ui-clearance;

$default-block-margin: $block-padding * 2 + $block-spacing; // This is the default margin between 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.

When you hover or select a block, we paint the outline outside the footprint of the block.

This means that if a block has no margin, these outlines will overlap. This variable is designed in a way so as to prevent this overlap from happening.

Copy link
Member

Choose a reason for hiding this comment

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

Minor: We should use parentheses so the future maintainer doesn't need to recall their order of operations 😄


// Buttons & UI Widgets
$button-style__radius-roundrect: 4px;
$button-style__radius-round: 50%;
45 changes: 38 additions & 7 deletions edit-post/assets/stylesheets/main.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ body.gutenberg-editor-page {
}

.gutenberg__editor {
// on mobile the main content area has to scroll
// otherwise you can invoke the overscroll bounce on the non-scrolling container, causing (ノಠ益ಠ)ノ彡┻━┻
// On mobile the main content area has to scroll, otherwise you can invoke
// the overscroll bounce on the non-scrolling container, causing (ノಠ益ಠ)ノ彡┻━┻.
@include break-small {
position: absolute;
top: 0;
Expand All @@ -102,11 +102,46 @@ body.gutenberg-editor-page {
min-height: calc( 100vh - #{ $admin-bar-height-big } );
}

// The WP header height changes at this breakpoint
// The WP header height changes at this breakpoint.
@include break-medium {
min-height: calc( 100vh - #{ $admin-bar-height } );
}

.components-navigate-regions {
height: 100%;
}
}

/**
* Vanilla editor styles
*/

.gutenberg__editor {

// Gutenberg specific elements
.editor-default-block-appender__content,
.components-placeholder {
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;
}

// Core 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.

This is not a complete list. Pushing the work in progress to decide if this approach is worth it at all.

.wp-block-button,
.wp-block-code,
.wp-block-categories,
.wp-block-cover-image,
.wp-block-embed {
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;
}

p,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should these rules be duplicated across each and every block in the style.scss file for each, or is it fine to keep these in a global vanilla theme like these?

ul,
ol {
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;
}

img {
max-width: 100%;
height: auto;
Expand All @@ -115,10 +150,6 @@ body.gutenberg-editor-page {
iframe {
width: 100%;
}

.components-navigate-regions {
height: 100%;
}
}

// Override core input styles to provide ones consistent with Gutenberg
Expand Down
10 changes: 0 additions & 10 deletions packages/editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,6 @@
.editor-block-list__block:first-child .editor-block-list__block-edit {
margin-top: $block-padding - $block-spacing / 2;
}

// Space every block, and the default appender, using margins.
// This allows margins to collapse, which gives a better representation of how it looks on the frontend.
.editor-default-block-appender__content,
.editor-block-list__block .editor-block-list__block-edit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we, currently in master, apply margins in a blanket statement to all blocks.

Why should we not do this?

Because some blocks might want their own margins, or no margins, and shouldn't have to override. Besides, the theme might not have any margins registered, and by registering them on a per-block basis, the theme will get these margins. This forces us to decide on a per block basis what, if any, margin is needed.

Copy link
Member

Choose a reason for hiding this comment

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

This forces us to decide on a per block basis what, if any, margin is needed.

But we apply a blanket rule to [ class*="wp-block-" ] { to apply those margins? Is that not contradicting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's definitely contradicting.

The thing is, I think of the Gutenberg editor style as a vanilla stylesheet. Same as if you create a stock HTML page with no CSS styles, you get the default browser stylesheet, with Times New Roman, etc.

Only, for margins, we currently set those on .editor-block-list__block-edit, as opposed to on the contents of the block itself. For example instead of setting a margin on a p tag, we set the margin on the .editor-block-list__block-edit container of the Paragraph block.

Why does this matter? Only one reason — if you mean to style the editor itself to match the theme, it makes more sense to write:

body.gutenberg-editor-page p,
body.gutenberg-editor-page ul,
body.gutenberg-editor-page ol,
... {
    margin: 1em 0;
}

... than it does to write:

body.gutenberg-editor-page .editor-block-list__block-edit {
    margin: 1em 0;
}

Sure, it would be nicer even if themers could write, simply:

p,
ul,
ol,
... {
    margin: 1em 0;
}

But unless Shadow DOM support drastically improves or we can put the editing canvas in an iframe, I doubt we can enable that. So by moving the margins to the block contents itself, at least we're taking a half-step to make styling the editor simpler, even if it's still the same margin.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I can support the goal, but as implemented, [ class*="wp-block-" ] { would still match exactly what we're targeting today with .editor-block-list__block-edit.

Sure, it would be nicer even if themers could write, simply:

In some side-chatting with @mtias and @youknowriad some ideas have been bounced around here with how we might be able to achieve effectively this by either rewriting the CSS to prepend necessary prefixing, or iterate them automatically when rendered in the DOM via document.styleSheets. Not sure if it'll be a foolproof solution, but worth exploring. I think @youknowriad said he might look into it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did say, and I'll give it a try, no updates at the moment though :(

Copy link
Contributor

@chrisvanpatten chrisvanpatten Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For what it's worth, you can never count on dynamic/SSR blocks to match a [class*="wp-block-"] selector. (Data point: none of our custom SSR blocks use wp-block- classes.)

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 can support the goal, but as implemented, [ class*="wp-block-" ] { would still match exactly what we're targeting today with .editor-block-list__block-edit.

While I agree that selector is not ideal or probably even feasible, it doesn't exactly target the same. I'm not trying to split hairs here, but look at the markup:

screen shot 2018-08-15 at 09 29 33

  • .editor-block-list__block is the parent wrapper of it all
  • .editor-block-list__block-edit is a direct descendant of that
  • .wp-block-quote is a direct descendant of .editor-block-list__block-edit

Only the third item is the same on the front-end and the back-end.

I realize that having the markup be the same in the frontend and the backend is likely not feasible in the near future, without Shadow DOM or things like that, but it is a baby step towrads at least styling the same bits of markup in the editor and the frontend. The margin is the same, sure, but the selector is different.

This all speaks to a larger issue that came up recently when @iseulde rewrote the table to use nested blocks. Essentially I wasn't able to write CSS that would make our nested blocks editor markup behave and look like the table on the front-end. Not easily at least.

I'm not sure what the best approach is — possibly this branch isn't it. But to your point about making a build process that matches front-end selectors to backend elements, this would not address the challenge for a table with nested blocks, where the sequence of parent-to-child elements in the markup is important (at least pending display: contents).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here you go #9008

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, you can never count on dynamic/SSR blocks to match a [class*="wp-block-"] selector. (Data point: none of our custom SSR blocks use wp-block- classes.)

@chrisvanpatten What class do those blocks have then? How would it be possible to style them?

.editor-block-list__layout .editor-block-list__block .editor-block-list__block-edit,
.editor-block-list__layout .editor-default-block-appender .editor-default-block-appender__content { // Explicitly target nested blocks, as those need to override the preceding rule.
margin-top: $block-padding * 2 + $block-spacing;
margin-bottom: $block-padding * 2 + $block-spacing;
}
}

.editor-block-list__layout .editor-block-list__block {
Expand Down