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

Try columns with grid and CSS vars #19297

Open
wants to merge 6 commits into
base: master
from

Conversation

@tellthemachines
Copy link
Contributor

tellthemachines commented Dec 23, 2019

Description

This is a proposal for drastically simplifying Columns/Column blocks styling with CSS grid and variables.

The main reason for this is getting rid of the need to set inline styles in blocks as much as possible, because inline styles break the resizable editor (#19082).

This work will incidentally also fix #18416 and #19112 / #19515.

  • Column width is now set in the columns block with CSS grid, but each column still computes its own width and may reset the width of the adjacent columns if total column widths exceed 100%.

  • Columns without an explicit width are set to 1fr, so remaining width is distributed equally among all columns without explicit widths.

  • I have added a basic flexbox fallback for IE, because this grid implementation depends on CSS variables to work. This means that IE users won't be able to preview columns with custom widths.

  • This setup is much easier to override by themes, because it doesn't depend as much on inline styles. E.g. themes can now cleanly override the grid-gap width with a single line of code.

How has this been tested?

Tested across browsers. IE maintains basic functionality.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@tellthemachines tellthemachines mentioned this pull request Dec 23, 2019
0 of 6 tasks complete
@aristath

This comment has been minimized.

Copy link

aristath commented Dec 23, 2019

hm.. It could also be as simple as

grid-template-columns: repeat(auto-fit, minmax(200px, 1fr));

That would simplify the code a lot and there would be no need for the css-var or calculating how many columns there are etc.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Jan 2, 2020

hm.. It could also be as simple as

grid-template-columns: repeat(auto-fit, minmax(200px, 1fr));

@aristath I'm afraid we can't use auto-fit or auto-fill here because we have to respect both the user-specified number of columns and the custom column widths, if they are set.

@tellthemachines tellthemachines marked this pull request as ready for review Jan 31, 2020
@tellthemachines tellthemachines force-pushed the try/columns-block-with-css-vars branch from f403d79 to fbb5227 Feb 3, 2020
@tellthemachines tellthemachines force-pushed the try/columns-block-with-css-vars branch from fbb5227 to c4d0a0d Feb 3, 2020
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Feb 3, 2020

This seems promising! I like the fact that unset columns mostly stay proportional.

In testing I had a few observations (tested using Twenty Nineteen):

  • I selected the layout with two equal columns and then tried setting the first to 50%. I would've expected there to be no change, but when explicitly setting the value to 50% the first column seems to be larger than the second. I wonder if the gaps between columns needs to be taken into account in the calculation.
  • Columns can expand beyond the block boundaries, particularly when there are 3 or more columns and one is set to a large size. I don't think this was possible before, or it wasn't as easy to achieve.
  • When setting one column width a value set in other columns can become unset. This seems to be the case when the total adds up to over 100%. This was particularly noticeable the more columns there are. An example is with four columns, if I set the first to 90% and then accidentally drag the second to over 10%, the first suddenly snaps back to being small, which is unexpected.
Copy link
Contributor

draganescu left a comment

This works good! 👏 Added just some minor comments.

packages/block-library/src/column/edit.js Outdated Show resolved Hide resolved
@@ -55,6 +43,7 @@ function ColumnEdit( {
<RangeControl
label={ __( 'Percentage width' ) }
value={ width || '' }
initialPosition={ 0 }

This comment has been minimized.

Copy link
@draganescu

draganescu Feb 3, 2020

Contributor

When is this visible?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Feb 5, 2020

Author Contributor

This should be visible when a column has no explicit width set.

packages/block-library/src/columns/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/columns/edit.js Outdated Show resolved Hide resolved
@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Feb 3, 2020

I selected the layout with two equal columns and then tried setting the first to 50%. I would've expected there to be no change, but when explicitly setting the value to 50% the first column seems to be larger than the second. I wonder if the gaps between columns needs to be taken into account in the calculation.

@talldan I tried this as well and the sizes remained the same. Although there is some flicker and i guess there remains an impression of the columns not being equal afterwards.

Also can we fix this weird state when the number next to the range control is empty?

Screenshot 2020-02-03 at 13 06 37

@tellthemachines tellthemachines requested review from nerrad and ntwb as code owners Feb 5, 2020
@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Feb 5, 2020

@talldan I have updated this to use fr units instead of %, because fr take into account the grid-gap so there should be no more weirdness in the column sizing 😅
I also changed the logic slightly to deal better with legacy column situations, where total sum of widths might exceed 100%. Exceeding 100% should no longer be possible with the changes in this PR.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Feb 6, 2020

@tellthemachines Hmm, I seem to end up with really narrow columns when adding the block for some reason:
Screen Shot 2020-02-06 at 11 26 26 am

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

tellthemachines commented Feb 6, 2020

Hmm, I seem to end up with really narrow columns when adding the block for some reason:

There were auto-margins on the inner columns. I've removed them and made some further margin-related tweaks. Everything should look as before now 😄

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

Successfully merging this pull request may close these issues.

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