Update page component (and some data view elements) spacing metrics #61333
Conversation
jameskoster
added
[Type] Enhancement
|
Size Change: +348 B (+0.02%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4145fe6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9209239438
|
|
Thanks for sweating these metrics. This is a tricky one. Three screenshots of the results:
Looking at each of those individually, on a big screen, they feel nice and harmonious. But switching between them, or looking at all three side by side in this overview context, things start to jump around. There's a bit of ways to go with some navigation sidebar metrics to change, but eventually there should be fewer gridlines than here: 
There should be some padding and positioning coherence between the two sides. That's thrown off here a little bit. Also, the Search field that looks a bit too small on the larger views:
A few thoughts:
Those are slightly longer term thoughts, and need not all be fixed in this PR, or even 6.6. For this one, I think we can probably do with the left/right padding changes, transition, and at least keeping the search field width list-view-only. What do you think? |
Yup let's try it.
Pushed this to try. I think it works quite well when switching from list to grid/table. But the opposite feels a bit uncanny to me. How do you feel about the grid spacing? Feel free to push any ideas :) |
There was a problem hiding this comment.
This experimental PR adjusts these metrics using CSS container queries which now have good support.
We might have to consider updating stylelint package to move this PR forward. My understanding is that current Gutenberg relies on stylelint version 14.2.0.
npm ls stylelint
[email protected] /home/username/projects/gutenberg
├─┬ @wordpress/[email protected] -> ./packages/scripts
│ └── [email protected]
└─┬ @wordpress/[email protected] -> ./packages/stylelint-config
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └── [email protected] deduped
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected] deduped
Additionally, it appears that stylelint version 14.12.0 fixed the container query warning (See this changelog).
So to avoid this warning and clear the lint check, I come up with the following approach.
- Add
stylelint-disablecomment to temporarily avoid the warning - Update the
stylelintversion to 14.12.0 or higher to avoid warning container queries - Update the
@wordpress/stylelint-configpackage to allow container queries
If we adopt the second and third approaches, I think it will affect various packages that depend on @wordpress/stylelint-config.
@youknowriad @gziolo, I am not familiar with package management, so if you have any good ideas, please let me know.
|
I added |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
How do you feel about this one @youknowriad? Specifically; should we update the stylelint package to allow container queries globally, keep it scoped down for now, or not allow it at all (abandon this PR)? |
| @@ -2,16 +2,20 @@ | |||
| color: $gray-800; | |||
| background: $white; | |||
| height: 100%; | |||
| container: edit-site-page / inline-size; /* stylelint-disable */ | |||
There was a problem hiding this comment.
The container queries are in the dataviews package and the container is outside, this is probably the only thing that I'd change, I'd move this to the root element of the dataviews probably.
There was a problem hiding this comment.
We need the container declaration on .edit-site-page so that the page header dimensions can adapt.
I suppose we could have another container declaration on .dataviews-wrapper, but I'm not sure it's worth it? 🤔
There was a problem hiding this comment.
I'm thinking we should only have in dataviews-wrapper no? The idea is to make these container styles work regardless of where we use the DataViews component. Isn't that possible?
There was a problem hiding this comment.
I guess the page-header is outside of the dataviews component. So yeah, maybe two containers.
There was a problem hiding this comment.
One way to check this is to test the dataviews in storybook and see if the spacing is correct there.
There was a problem hiding this comment.
Oh good point. Yes we'll probably need two containers.
|
I think the basic usage of container queries is now allowed in all the browsers that we support. So yeah, we should consider updating stylelint and moving forward if this is deemed an improvement over trunk. |
Should we update stylelint in this PR or separately? |
|
stylelint upgrade is probably better done separately. |
packages/dataviews/src/style.scss
Outdated
| @@ -1,13 +1,18 @@ | |||
| :root { | |||
| --dataviews-padding-x: #{$grid-unit-60}; | |||
There was a problem hiding this comment.
What I don't like about random CSS variables is that they kind of become official public APIs. Is there a way to achieve the same with SASS variables or something?
There was a problem hiding this comment.
I don't think it's possible to do this with sass vars.
|
0.1 works for me. |
packages/dataviews/src/style.scss
Outdated
| @@ -3,17 +3,19 @@ | |||
| overflow: auto; | |||
| box-sizing: border-box; | |||
| scroll-padding-bottom: $grid-unit-80; | |||
| container: dataviews-wrapper / inline-size; /* stylelint-disable -- '@container' not globally permitted */ | |||
There was a problem hiding this comment.
I think that with this comment, all stylelint rules will be disabled in the code that follows. I think it is better to use stylelint-disable-next-line instead of stylelint-disable and further limit the rules to be disabled. The same goes for other files.
.dataviews-wrapper {
/* stylelint-disable-next-line property-no-unknown -- '@container' not globally permitted */
container: dataviews-wrapper / inline-size;
}
/* stylelint-disable-next-line scss/at-rule-no-unknown -- '@container' not globally permitted */
@container (max-width: 430px) {
}There was a problem hiding this comment.
Oh you're right!
|
@cleacos @madebynoam @lucasmendes-design @jeffgolenski Heads-up that the spacing in data views components is bound to change with this PR. Let's check how these changes affect both Dotcom & A4A in the long run. |
packages/dataviews/src/style.scss
Outdated
| .dataviews-filters__reset-button[aria-disabled="true"] { | ||
| &, | ||
| &:hover { | ||
| opacity: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
Is there a reason this was added? This makes the reset button look cramped.
There was a problem hiding this comment.
Good spot, it was to reduce the space occupied by the button to avoid wrapping. That should probably be looked at separately though.
packages/dataviews/src/style.scss
Outdated
| padding: $grid-unit-15 $grid-unit-40; | ||
| padding: $grid-unit-20 $grid-unit-60; |
There was a problem hiding this comment.
I think the vertical padding needs to be maintained. Otherwise, it will be out of alignment with the sidebar border.
| padding: 0 $grid-unit-40; | ||
| padding: 0 $grid-unit-60; |
There was a problem hiding this comment.
I think it would be better to consider container queries for this padding as well. Otherwise, the left and right padding will not match when matching the container query.
| @@ -100,7 +100,7 @@ | |||
| .edit-site-patterns__section-header { | |||
There was a problem hiding this comment.
I think we need to add a transition (and reduce-motion) here as well.
931ee9d04537465594b70c3cbbc73959.mp4
|
Thanks for the thorough review @t-hamano :) |
|
I suspect we may end up tweaking (or expanding the scale) for wider widths down the road, but I think this is worth merging for 6.6 because it adds some polish to the new List layout. |
…ordPress#61333) Co-authored-by: jameskoster <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: keoshi <[email protected]>
…ordPress#61333) Co-authored-by: jameskoster <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: keoshi <[email protected]>
| &:first-child, | ||
| &:last-child { | ||
| transition: padding ease-out 0.1s; | ||
| @include reduce-motion("transition"); |
There was a problem hiding this comment.
Do you remember why this animation was added @jameskoster ? It's causing some weird effects when sorting / filtering...
There was a problem hiding this comment.
It was a suggestion by @jasmussen to avoid an instantaneous jump when the padding increased / decreased as you resize the viewport.
Not essential imo.
There was a problem hiding this comment.
Yep, can remove if it causes issues.
| @@ -809,6 +825,38 @@ | |||
| } | |||
| } | |||
|
|
|||
| /* stylelint-disable-next-line scss/at-rule-no-unknown -- '@container' not globally permitted */ | |||
| @container (max-width: 430px) { | |||
There was a problem hiding this comment.
@jameskoster @youknowriad @jasmussen
$break-mobile: 480px instead of the hard-coded 430px? Currently, 430px is hard-coded outside of DataViews, which means third-party apps using DataViews might need to replicate this hard-coding if they want to align their padding with DataViews (an example).
If not, do you have plans to define 430px as a reusable variable? Thank you!
There was a problem hiding this comment.
The question seems to be what makes sense for DataViews component when used in isolation (storybook can be a good way to think about this).
There was a problem hiding this comment.
Right, I think the instinct to find a rule that makes this value feel less arbitrary is valid enough in terms of the component system. But honestly I'd love to move all of the UI towards something better than the 480/600/782px values that currently make up the breakpoints. Those seem perhaps even more arbitrary.
There was a problem hiding this comment.
I think this should probably be informed by whatever we come up with in #53617.
The spacing values in the page component and many data views elements are a "one size fits all", which of course doesn't really fit all, and results in compromises. For example in narrow instances (like List layout) the spacing feels a bit too large. In wider instances (grid / table layouts) you could say the spacing feels too small, especially on very large screens.
This experimental PR adjusts these metrics using CSS container queries which now have good support.
The values are not necessarily proposals, just a demonstration of what we might do. I'm drafting this PR initially to discuss the merits of this approach, and whether it's something we want to pursue, and if so how we might configure things.