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

Block Library: Columns: Force 50% column width at mid-range viewport #20597

Merged
merged 1 commit into from Mar 2, 2020

Conversation

@aduth
Copy link
Member

@aduth aduth commented Mar 2, 2020

Fixes #18416

This pull request seeks to resolve an issue where columns do not always size correctly at mid-range viewports. Specifically, columns with explicit widths assigned will assign those widths using an inline style attribute which, prior to these changes, would take precedent over the 50% intended width at this viewport range.

These changes resolve this by assigning the flex-basis using !important. While !important is typically discouraged, it is the only means by which the inline style can be overridden, which is the intent of these styles (to override the explicit assigned width). An alternative would require that the flex-basis be assigned by some other means, e.g. a combination of data- attribute and CSS attr(), which has no browser support and would not be backwards-compatible with existing columns content.

Note that there is already prior art for this behavior. The styles which force 100% width in mobile viewports is applied in the exact same fashion:

@media (max-width: #{ ($break-small - 1) }) {
// Responsiveness: Show at most one columns on mobile. This must be
// important since the Column assigns its own width as an inline style.
flex-basis: 100% !important;
}

The important caveat is that unless this is used with max-width media query, then the !important styles would override the intended explicit widths at larger viewports. That is the reason for changing from using the mixin to a manually-crafted @media query (same as with the mobile styles).

Note about WordPress 5.4 cherry-picking: It was my hope to try to have this fixed in time for WordPress 5.4, despite being a bug which has existed for some time. While it could still be included, it is complicated by the facts that (a) RC1 is scheduled for tomorrow, and since this is not a regression of WordPress 5.4, it may not qualify for cherry-picking during RC, and (b) the patch would not apply cleanly because of refactoring which has occurred in this stylesheet since 5.4-beta1 (#20529).

Testing Instructions:

Make a post including Columns with varying combinations of explicit and flexible-width columns. Specifically, you should observe that when assigning an explicit width to a column, the width is overridden when previewed at viewport sizes between ~600-800px.

Repeat steps to reproduce from #18416

@github-actions
Copy link

@github-actions github-actions bot commented Mar 2, 2020

Size Change: +23 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-library/style-rtl.css 7.51 kB +11 B (0%)
build/block-library/style.css 7.51 kB +12 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 105 kB 0 B
build/block-editor/style-rtl.css 10.5 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/editor-rtl.css 7.4 kB 0 B
build/block-library/editor.css 7.4 kB 0 B
build/block-library/index.js 116 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.53 kB 0 B
build/edit-post/style.css 8.53 kB 0 B
build/edit-site/index.js 4.63 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.42 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 780 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

// As with mobile styles, this must be important since the Column
// assigns its own width as an inline style, which should take effect
// starting at `break-medium`.
flex-basis: calc(50% - #{$grid-unit-20}) !important;
flex-grow: 0;

This comment has been minimized.

@aduth

aduth Mar 2, 2020
Author Member

Note that there are two other styles here that will now only take effect between $break-small and $break-medium - 1:

  • flex-grow
  • :nth-child( even ) { margin-left }

As far as I see it, this is not only fine, but in-fact preferable, since these styles are specific to how the columns are presented in the mid-range viewport (with 50% width).

Practically speaking, it should have no effective difference either, since:

  • flex-grow is overridden at break-medium
  • margin-left is assigned to the same value $grid-unit-20 * 2 at break-medium for :not(:first-child), which includes (is a superset of) :nth-child(even)

This comment has been minimized.

@jasmussen

jasmussen Mar 3, 2020
Contributor

Code looks good, if complex :)
Thank you for doing this.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

The change seems to work as expected.
Before:
image

After:
image

@aduth
Copy link
Member Author

@aduth aduth commented Mar 2, 2020

@jorgefilipecosta FWIW I was mostly expecting this to have an impact on front-end of the previewed post. I guess if the stylesheet is loaded in the editor too, it may have a benefit there as well.

@aduth aduth merged commit 34adcaf into master Mar 2, 2020
4 checks passed
4 checks passed
@github-actions
build
Details
@github-actions
pull-request-automation
Details
@github-actions
pull-request-automation
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the fix/18416-columns-wrap branch Mar 2, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 2, 2020
@dianeco
Copy link

@dianeco dianeco commented Mar 6, 2020

Hi there,
Before this change, it was possible, for example, to keep a 20% 80% column layout on medium devices. Now, it automatically resizes to 50% 50%, which isn't suitable for some layouts. Is there a way to let the user choose or override this?

@aduth
Copy link
Member Author

@aduth aduth commented Mar 6, 2020

Hi @dianeco , my understanding is that the intended behavior was always that there would be two equal-width columns shown at mid-range viewports. This didn't work correctly before, resulting in a behavior such as the one you describe, but also resulting in issues like that summarized in #18416 (fixed by these changes).

Based on some previous discussion in #20169 (specifically #20169 (review) and related work in #19909), I believe there might be some future desire to remove this mid-range behavior altogether, or at least to allow the user to have granular control over the column widths at various viewport sizes.

cc @jasmussen

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Mar 9, 2020

I would agree that allowing users to adjust not only the specific amount of columns at mobile tablet and desktop breakpoints, but their individual widths as well, is something we should endeavour to support!

However it's a task that is perhaps best accomplished in a way that lets you customize ANY block and those breakpoints, not just the Columns block specifically. #19909 was created to enable such customizations.

@dianeco
Copy link

@dianeco dianeco commented Mar 9, 2020

@jasmussen #19909 seems very promising. But in the meantime, as it was possible before this change to keep an unequal columns layout on medium devices (with some CSS changes), would you consider creating a class that the user can add to avoid the 50% resizing.

For example, replacing the current code

@media (min-width: 600px) and (max-width:781px) {
	.wp-block-column {
	    flex-basis: calc(50% - 16px)!important;
	    flex-grow: 0;
	}
}

by

@media (min-width: 600px) and (max-width:781px) {
	.wp-block-columns:not(.cols-not-resizing) > .wp-block-column {
	    flex-basis: calc(50% - 16px)!important;
	    flex-grow: 0;
	}
}

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Mar 9, 2020

Such a class would be edgecase CSS to support a feature meant to be replaced.

However it would be nice if we could replace that !important modifier, that way you could at least add a custom CSS class to your column in the advanced section:

Screenshot 2020-03-09 at 12 52 41

That way you should be able to customize those breakpoints, no?

@dianeco
Copy link

@dianeco dianeco commented Mar 9, 2020

Yes, if we could replace the !important modifier, it would be easy to override the styling. But with the inline style used when an explicit width is assigned to a column, I don't see how it's possible to remove the !important modifier.

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Mar 9, 2020

Any thoughts on whether we can find a way to remove that !important, @aduth?

@aduth
Copy link
Member Author

@aduth aduth commented Mar 9, 2020

The !important is necessary based on how the user-assigned width is applied using an inline style tag (related). The only alternatives I can think of are:

  • Avoiding inline styles in favor of a limited subset of width arrangement classes, e.g. is-width-1-percent, is-width-2-percent, or is-width-one-third. This limits the control over assigning width in a way at odds with how the columns are currently implemented, and in a way I'm not sure would be desirable.
  • If there were better browser support for it, it could be possible to do some combination of data- attribute and styling in the stylesheet (e.g. flex-basis: attr( 'data-width' % );). We're limited by browser support here.

But between the comments of #20597 (comment) and #20597 (comment), it seems to me it should already be possible to override this by assigning an additional class name to the column and using a custom style which both has high specificity (higher than the base style) and !important? I expect that could allow the style to be overridden.

@dianeco
Copy link

@dianeco dianeco commented Mar 9, 2020

Yes, it's already possible to override style with higher specificity and !important. The difficulty is that I need a class that keeps the columns width on medium devices, and that covers all cases, no matter the value of the column width. I achieve it using the code below but was hoping for a simpler solution 🙂.

@media (min-width: 600px) and (max-width:781px) {
	.wp-block-columns.cols-not-resizing > .wp-block-column[style*="flex-basis:5%"] {
	    flex-basis: 5% !important;
	}
	.wp-block-columns.cols-not-resizing > .wp-block-column[style*="flex-basis:6%"] {
		flex-basis: 6% !important;
	}
	.wp-block-columns.cols-not-resizing > .wp-block-column[style*="flex-basis:7%"] {
		flex-basis: 7% !important;
	}
       
        [...]
}

@ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Mar 9, 2020

I just installed WordPress 5.4 RC1 (and disabled the Gutenberg plugin), and I can confirm that #18416 is fixed! 😄

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

Successfully merging this pull request may close these issues.

5 participants