-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Grid.Cell] Add column, and row props #6055
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
Conversation
size-limit report 📦
|
8aa7c1a to
fe3ebdb
Compare
fe3ebdb to
500e443
Compare
| --pc-row-xs: initial; | ||
| --pc-row-sm: var(--pc-row-xs); | ||
| --pc-row-md: var(--pc-row-sm); | ||
| --pc-row-lg: var(--pc-row-md); | ||
| --pc-row-xl: var(--pc-row-lg); | ||
| --pc-column-xs: initial; | ||
| --pc-column-sm: var(--pc-column-xs); | ||
| --pc-column-md: var(--pc-column-sm); | ||
| --pc-column-lg: var(--pc-column-md); | ||
| --pc-column-xl: var(--pc-column-lg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is pretty confusing. Are we setting everything to initial? I don't see a lot of values in the --pc- approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's hard to understand at first but I think it's better than doing this even though that might be easier to understand?
It was @aaronccasanova 's suggestion and I liked it better than calling var() a ton of times in what is essentially just as hard to read css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so you only have to set things like row, column, grid-template-areas at xs and lg as a minimum vs having to set them all. xs = 6 column, lg is where we switch to 12 and they cascade from a mobile up perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-page See this comment and associated demo of the behavior:
#5966 (comment)
As Kyle mentioned, this enables the mobile first props strategy used throughout the component.
Before:
<Grid columns={{xs: 4, sm: 4, md: 4, lg: 6, xl: 6}} />After:
<Grid columns={{xs: 4, lg: 6}} />There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we leave a little comment above this css that explains what it is doing. It makes sense to me but took a while to understand what is going on.
Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes