Skip to content

fix: fix XY Grid responsive widths with shrink/auto #10891 #10927

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

Jump to bottom
Merged
merged 2 commits into from
Mar 3, 2018
Merged

fix: fix XY Grid responsive widths with shrink/auto #10891 #10927

merged 2 commits into from
Mar 3, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Feb 10, 2018 •
edited
Loading

Changes:

  • add missing flex reset in XY Grid responsive cell widths
  • add visual test for XY Grid reponsive widths

Closes #10891

@ncoden ncoden added this to the 6.4.4 milestone Feb 10, 2018
@ncoden ncoden mentioned this pull request Feb 10, 2018
Closed
@ncoden
Copy link
Contributor Author

ncoden commented Feb 26, 2018

@DanielRuf @JeremyEnglert @brettsmason Could one of you review this ?

@DanielRuf DanielRuf self-requested a review February 26, 2018 07:28
DanielRuf
DanielRuf approved these changes Feb 28, 2018
View reviewed changes
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

LGTM, fixes the codepen examples.

@ncoden
Copy link
Contributor Author

ncoden commented Feb 28, 2018

Thanks @DanielRuf.

With changes like that (adding a mixin somewhere), you can also check the diff in the generated CSS, to ensure that:

  1. this does not add too much CSS.
  2. this is the most efficient way to solve the problem.
  3. this does not introduce specificity issues.

@DanielRuf
Copy link
Contributor

But still wondering why shrink is needed here in the mixin. 🤔

@ncoden
Copy link
Contributor Author

ncoden commented Feb 28, 2018 •
edited
Loading

See: scss/xy-grid/_cell.scss:58

This is the flex behavior we expect to get static width: do not grow, do not shrink, simply takes the width applied to it. Like at scss/xy-grid/_cell.scss:51

@DanielRuf
Copy link
Contributor

A bit confusing at first with the shrink option but now I fully understand.

@ncoden ncoden merged commit 9234969 into foundation:develop Mar 3, 2018
@ncoden ncoden deleted the fix/xy-grid-responsive-modifiers-reset-10891 branch April 7, 2018 17:46
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
@ncoden
Use pull request foundation#10927 from ncoden/fix/xy-grid-responsive-…
620632e 
…modifiers-reset-10891 for v6.5.0

8347330 fix: add missing flex reset in XY Grid responsive cell widths foundation#10891
16e18da tests: add visual test for XY Grid reponsive widths foundation#10891

Signed-off-by: Nicolas Coden <[email protected]>
This was referenced Jun 16, 2018
Merged
Merged
@ncoden ncoden mentioned this pull request Jul 10, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRuf DanielRuf DanielRuf approved these changes

@brettsmason brettsmason Awaiting requested review from brettsmason

@JeremyEnglert JeremyEnglert Awaiting requested review from JeremyEnglert

Assignees
No one assigned
Labels
bug fix XY Grid
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ncoden @DanielRuf