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

Remove template locking from the columns block #20465

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

youknowriad
Copy link
Contributor

With all the new UI updates, I don't think there's a valid reason to keep the template locking in the Columns block. What this means concretely is that the columns block becomes just a list of "column" block, you can remove/add any columns you want and also use the block movers (horizontal) from the toolbar to move columns.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Block] Columns Affects the Columns Block labels Feb 26, 2020
@youknowriad youknowriad requested a review from aduth February 26, 2020 12:45
@youknowriad youknowriad self-assigned this Feb 26, 2020
@github-actions
Copy link

Size Change: -45 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-library/editor-rtl.css 7.64 kB -21 B (0%)
build/block-library/editor.css 7.64 kB -21 B (0%)
build/block-library/index.js 116 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1 kB 0 B
build/annotations/index.js 3.43 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 104 kB 0 B
build/block-editor/style-rtl.css 10.3 kB 0 B
build/block-editor/style.css 10.3 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.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 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 90.9 kB 0 B
build/edit-post/style-rtl.css 8.59 kB 0 B
build/edit-post/style.css 8.58 kB 0 B
build/edit-site/index.js 4.62 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.41 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 4.01 kB 0 B
build/editor/style.css 4 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 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.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 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.68 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.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 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.49 kB 0 B
build/priority-queue/index.js 879 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 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

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise, this makes sense to me. I believe one of the original reasons for the locking was due to the lack of a horizontal mover, and the vertical movers not making much sense in this context. Since that the horizontal mover functionality exists, it seems reasonable to me to make this change.

Comment on lines -12 to -18
// Ideally all block toolbars should be positioned the same.
.components-popover.block-editor-block-list__block-popover
.components-popover__content
.block-editor-block-contextual-toolbar[data-type="core/column"] {
margin-left: 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

What was the purpose of these 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.

I don't know @jasmussen maybe, It was definitely breaking the column block toolbar position though.

Copy link
Contributor

Choose a reason for hiding this comment

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

These styles were almost certainly necessary due to the complex left/right margins that innerblock containers used to be born with.

But it's easy to test, if the toolbar is positioned correctly according to the block selected, then these styles are not needed anymore. And good riddance!

@youknowriad youknowriad merged commit 1fb9868 into master Feb 27, 2020
@youknowriad youknowriad deleted the update/remove-locking-from-columns branch February 27, 2020 08:31
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 27, 2020
@ellatrix ellatrix mentioned this pull request Feb 28, 2020
6 tasks
@oxyc oxyc mentioned this pull request Jun 19, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants