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: Styling problem on vertically aligned blocks #20368

Merged

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Feb 21, 2020

Description

Fixes: #19962

Props to @mrleemon for suggesting this fix.

Some blocks did not work inside the columns block, this PR adds a CSS fix for the issue.
In my tests, the fix does not seem to have unexpected consequences.

cc: @aduth, @jasmussen, @getdave I would appreciate if you could double-check if this fix works as expected.

How has this been tested?

I added a columns block with two columns.
I added the Image Slider Block from the "Ultimate Blocks" plugin inside with some images inside the first column.
I added sample text on the second column making the height of the second column higher than the one of the first column.
I vertically aligned the columns to the three possible values and verified for each alignment the result looked correct. (On master the slider is broken).

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Feb 21, 2020

Size Change: +27 B (0%)

Total Size: 864 kB

Filename Size Change
build/block-library/style-rtl.css 7.49 kB +14 B (0%)
build/block-library/style.css 7.49 kB +13 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 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 9.78 kB 0 B
build/block-editor/style.css 9.77 kB 0 B
build/block-library/editor-rtl.css 7.67 kB 0 B
build/block-library/editor.css 7.67 kB 0 B
build/block-library/index.js 114 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 190 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 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.04 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.7 kB 0 B
build/edit-post/style-rtl.css 8.7 kB 0 B
build/edit-post/style.css 8.69 kB 0 B
build/edit-site/index.js 4.58 kB 0 B
build/edit-site/style-rtl.css 2.77 kB 0 B
build/edit-site/style.css 2.76 kB 0 B
build/edit-widgets/index.js 4.36 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 327 B 0 B
build/editor/editor-styles.css 328 B 0 B
build/editor/index.js 45.1 kB 0 B
build/editor/style-rtl.css 4.13 kB 0 B
build/editor/style.css 4.11 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 500 B 0 B
build/format-library/style.css 501 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.45 kB 0 B
build/is-shallow-equal/index.js 711 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 215 B 0 B
build/list-reusable-blocks/style.css 216 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 878 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

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Feb 24, 2020

In my quick test, this does appear to work as intended with no side-effects. Nice work! 👍 👍

@jorgefilipecosta jorgefilipecosta merged commit bef4179 into master Feb 24, 2020
4 checks passed
4 checks passed
build
Details
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 24, 2020
@jorgefilipecosta jorgefilipecosta deleted the fix/style-problem-on-vertically-aligned-columns branch Feb 24, 2020
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 24, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 25, 2020

I'm a bit confused what impact width has to have fixed the original issue. Related to that, it's not entirely clear to me why align-self: center would have caused #19962 in the first place (and whether or not this is an issue of the core column block, or the implementation of the affected custom blocks).

I might also be a little concerned how well an explicit width interoperates with the default flex styling of the column. There's a lot of potential edge cases with mobile, tablet, desktop column growth and with or without explicit widths on columns. @jasmussen tends to be pretty well-versed in testing these edge cases, so if he hadn't found an issue, it may not be a problem. To a more general question, I'm not entirely clear on how width is taken into consideration when an element otherwise uses flex-basis and flex-grow.

@mrleemon

This comment has been minimized.

Copy link
Contributor

mrleemon commented Feb 25, 2020

If it is of any use, another side-effect of NOT adding width: 100% to vertically-aligned columns is that the block inserter gets shrinked and is shifted to the left, center or right depending on the align-self value.

columns-none
columns-top
columns-center
columns-bottom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.