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

Revert "Fix: Remove unrequired margins from the columns block" #21824

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Apr 23, 2020

Reverts #21615

See #21615 (comment).

I added a commit here applying margins to the placeholder.

@github-actions
Copy link

github-actions bot commented Apr 23, 2020

Size Change: +36 B (0%)

Total Size: 845 kB

Filename Size Change
build/block-library/editor-rtl.css 7.14 kB +14 B (0%)
build/block-library/editor.css 7.14 kB +15 B (0%)
build/block-library/index.js 112 kB +7 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.1 kB 0 B
build/block-editor/style.css 10.1 kB 0 B
build/block-library/style-rtl.css 7.19 kB 0 B
build/block-library/style.css 7.19 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 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 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 28.2 kB 0 B
build/edit-post/style-rtl.css 12.5 kB 0 B
build/edit-post/style.css 12.5 kB 0 B
build/edit-site/index.js 10.8 kB 0 B
build/edit-site/style-rtl.css 5.25 kB 0 B
build/edit-site/style.css 5.24 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 5 kB 0 B
build/edit-widgets/style.css 5 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 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.32 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.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 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

@@ -234,7 +234,7 @@ const ColumnsEdit = ( props ) => {
}

return (
<Block.div>
<Block.div className="has-placeholder">
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this class, can you target the placeholder component directly? Something like

[data-type="core/columns"] > .components-placeholder {
	margin.... 
}

That would also allow us to avoid the :not selector below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try this, but the block would need to be aware of the variable. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing some of the context here, so I don't want to give bad advace. In fact I'd defer to your self and @jorgefilipecosta on the technical details — it just seemed like an additional class felt too much. If it isn't, ignore me!

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @ellatrix thank you for submitting a fix to the issue 👍

Now I understand why the margins are needed but making some tests in this PR I notice some undesired behaviors:

If add two columns block following each other I get the following result:
image
When I add two group blocks I get the following:
image
Should the consecutive columns look similar to the consecutive groups?

On the widgets screen with these changes, the block UI appears above the name of the widget area:
image

On the widgets screen, we lose the space between the columns block and the inserter:
image

All the cases happen when the columns already contains column blocks but the each column itself is empty.

@ellatrix
Copy link
Member Author

@jasmussen Now I'm doubting if it should be done like this... because I see the following styles:

// This is the style used on the front-end, which ideally should be loaded in
// the editor too.
.wp-block-column > *:first-child {
	margin-top: 0 !important;
}

.wp-block-column > *:last-child {
	margin-bottom: 0 !important;
}

@jasmussen
Copy link
Contributor

I created #22018 as an alternative, thanks Ella your last comment put me on that trail. What do you think?

@ellatrix ellatrix closed this May 4, 2020
@aristath aristath deleted the revert-21615-fix/remove-unrequired-margins-from-the-columns-block branch November 10, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants