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.

Sign up for GitHub

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

Update page component (and some data view elements) spacing metrics #61333

Merged
merged 23 commits into from
May 24, 2024
+85 −15
Merged

Update page component (and some data view elements) spacing metrics #61333

Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d45b50b
Update metrics
jameskoster Apr 24, 2024
975b3b2
vars
jameskoster Apr 24, 2024
afc52a1
A different approach
jameskoster Apr 24, 2024
6f15aa3
pagination
jameskoster Apr 24, 2024
dae0d7b
List layout
jameskoster Apr 26, 2024
9dcfa7c
Merge branch 'trunk' into update/page-metrics
jameskoster May 2, 2024
8599ed4
filter, grid spacing
jameskoster May 2, 2024
3c7a9c7
rebase
jameskoster May 2, 2024
12a916f
list item padding
jameskoster May 2, 2024
756bb45
Further adjustments
jameskoster May 3, 2024
6d27cf4
disable stylelint for container queries
jameskoster May 9, 2024
0292095
Merge branch 'trunk' into update/page-metrics
jameskoster May 9, 2024
244ffd6
Rebase
jameskoster May 17, 2024
1000a17
Add description to stylelint-disable
jameskoster May 17, 2024
94806b1
Add container, update table + grid
jameskoster May 21, 2024
122a91a
Use css vars
jameskoster May 21, 2024
d8ea7e4
rebase
jameskoster May 21, 2024
f466e32
Revert use of css vars
jameskoster May 21, 2024
efb40c9
Fix patterns page header
jameskoster May 21, 2024
7f7de3b
Reduce animation speed
jameskoster May 21, 2024
b17ba83
Update stylelint disabling
jameskoster May 22, 2024
4145fe6
Address feedback
jameskoster May 23, 2024
beeef7b
Merge branch 'trunk' into update/page-metrics
jameskoster May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
:root {
--dataviews-padding-x: #{$grid-unit-60};
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 think it's possible to do this with sass vars.

}

.dataviews-wrapper {
height: 100%;
overflow: auto;
box-sizing: border-box;
scroll-padding-bottom: $grid-unit-80;
container: dataviews-wrapper / inline-size; /* stylelint-disable -- '@container' not globally permitted */
Copy link
Contributor

@t-hamano t-hamano May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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) {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're right!

}

.dataviews-filters__view-actions {
box-sizing: border-box;
padding: $grid-unit-20 $grid-unit-60;
padding: $grid-unit-20 var(--dataviews-padding-x);
flex-shrink: 0;
position: sticky;
left: 0;
Expand Down Expand Up @@ -44,7 +49,7 @@
bottom: 0;
left: 0;
background-color: $white;
padding: $grid-unit-20 $grid-unit-60;
padding: $grid-unit-20 var(--dataviews-padding-x);
border-top: $border-width solid $gray-100;
color: $gray-700;
flex-shrink: 0;
Expand Down Expand Up @@ -104,7 +109,7 @@

td:first-child,
th:first-child {
padding-left: $grid-unit-60;
padding-left: var(--dataviews-padding-x);

.dataviews-view-table-header-button,
.dataviews-view-table-header {
Expand All @@ -114,7 +119,7 @@

td:last-child,
th:last-child {
padding-right: $grid-unit-60;
padding-right: var(--dataviews-padding-x);
}

&:last-child {
Expand Down Expand Up @@ -283,7 +288,7 @@
margin-bottom: $grid-unit-30;
grid-template-columns: repeat(2, minmax(0, 1fr)) !important;
grid-template-rows: max-content;
padding: 0 $grid-unit-60;
padding: 0 var(--dataviews-padding-x);

@include break-xlarge() {
grid-template-columns: repeat(3, minmax(0, 1fr)) !important; // Todo: eliminate !important dependency
Expand Down Expand Up @@ -546,7 +551,7 @@

.dataviews-no-results,
.dataviews-loading {
padding: 0 $grid-unit-60;
padding: 0 var(--dataviews-padding-x);
flex-grow: 1;
display: flex;
align-items: center;
Expand Down Expand Up @@ -804,10 +809,9 @@
}
}

@container (max-width: 430px) { /* stylelint-disable -- '@container' not yet globally permitted */
.dataviews-pagination,
.dataviews-filters__view-actions {
padding: $grid-unit-20 $grid-unit-30;
@container (max-width: 430px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jameskoster @youknowriad @jasmussen

badge Would it make sense to update this to $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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 think this should probably be informed by whatever we come up with in #53617.

* {
--dataviews-padding-x: #{$grid-unit-30};
}

.dataviews-filters__view-actions {
Expand Down
Loading