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

[@mantine/core] Grid: Fix offset and order responsive props #2163

Merged
merged 1 commit into from Aug 20, 2022
Merged

[@mantine/core] Grid: Fix offset and order responsive props #2163

merged 1 commit into from Aug 20, 2022

Conversation

mdodell
Copy link
Contributor

@mdodell mdodell commented Aug 18, 2022

Fixes issue with both offset and order props for Grid.Col that requires each to have a responsive prop (xs, sm, md, lg, xl) for their respective responsive props (offsetXs, orderXs, etc). to work.

Also updated the docs for order to properly use Grid.Col rather than the Col shorthand.

Comment on lines +57 to +61
maxWidth: grow
? 'unset'
: typeof sizes[size] === 'number'
? getColumnWidth(sizes[size], columns)
: 'none',
Copy link
Contributor Author

@mdodell mdodell Aug 18, 2022

Choose a reason for hiding this comment

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

Not super in love with the double ternary statement so open to feedback here. If the sizes[size] isn't defined, we just use the initial value for that CSS property (none in the case of max-width and auto in the case of flexBasis).

@rtivital rtivital merged commit 0326f9b into mantinedev:master Aug 20, 2022
@rtivital
Copy link
Member

Thanks!

@rtivital
Copy link
Member

@mdodell this PR broke all Grid responsive styles, I've reverted it

rtivital added a commit that referenced this pull request Aug 22, 2022
@mdodell
Copy link
Contributor Author

mdodell commented Aug 22, 2022

@mdodell this PR broke all Grid responsive styles, I've reverted it

Interesting. I'll submit a follow-up PR, I think that the check for maxWidth and flexBasis was breaking it.

@rtivital
Copy link
Member

No need, I've already resolved the isuue

@mdodell
Copy link
Contributor Author

mdodell commented Aug 22, 2022

No need, I've already resolved the isuue

Ah - perfect. My apologies here. Is it now resolved so that the MantineSize prop is no longer needed in order to check for order? I just fixed it locally before I saw this comment - it looks like maxWidth and flexBasis just both need to be set to 100% if the typeof sizes[size] check is falsey.

It looks like currently the values rae being set to NAN% - which I think ends up setting them to 100% - potentially through other calculations done somewhere else in the CSS scope?

nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 22, 2022
nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 22, 2022
nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 28, 2022
nmay231 pushed a commit to nmay231/mantine that referenced this pull request Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants