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

fix(grid): remove column flex styles #303

Merged
merged 4 commits into from
Feb 11, 2020
Merged

Conversation

richbachman
Copy link
Contributor

@SiTaggart discovered we were using flex improperly in Column. Flex was interfering with children block elements an not allowing them to be full width. The flex styles are unnecessary because we're calculating a width for each Column.

This removes the Column flex styles, which allows children block elements to be 100% width.

@richbachman richbachman added Type: Bug Something isn't working Status: Pls CR This PR is ready for Code Reviews Area: Components Related to the component library (core) of this system labels Feb 11, 2020
@vercel
Copy link

vercel bot commented Feb 11, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/twilio-dsys/paste/gctx3i8ya
✅ Preview: https://paste-git-fix-gridremove-column-flex.twilio-dsys.now.sh

@SiTaggart
Copy link
Contributor

Do you mind removing all the width 100% on the box components in your stories to prove the theory?

@TheSisb
Copy link
Contributor

TheSisb commented Feb 11, 2020

Curious why this wasn't caught in any tests. Can we add a test for it or a storybook story of the behavior?

@richbachman
Copy link
Contributor Author

@SiTaggart stories updated and theory proven. When I added the display flex rule, it set the default value of flex-wrap to nowrap, which overrides the width of block elements unless those elements have a width for flex-basis set.

@richbachman
Copy link
Contributor Author

@TheSisb it was an error on my part. In the tests, we're not adding any child elements to each Column so the error wouldn't really have shown up. Stories are now updated so we can visually see the block child elements are now full width of each Column.

@SiTaggart
Copy link
Contributor

Worth adding a couple of cards in grid stories? That would prove our compositions don’t affect our own components in adverse ways, rather than faking examples with box all the time?

Setting 100% width and not type checking mdx and storybook files is biting us a little

@richbachman
Copy link
Contributor Author

@SiTaggart I've added a couple of stories showing actual components.

@richbachman richbachman merged commit 266b31d into master Feb 11, 2020
@richbachman richbachman deleted the fix/grid/remove-column-flex branch February 11, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Components Related to the component library (core) of this system Status: Pls CR This PR is ready for Code Reviews Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants