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

feat(react): add unstable_Grid components #3892

Closed

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Sep 2, 2019

Add layout helpers for managing a Grid alongside the Carbon Design System layout guidance. Namely, this PR adds support for Grid, Row, and Column components. Grid and Row are roughly the same layout containers, supporting various modes for "condensed" or "no gutter' moments. Column provides support for specifying responsive breakpoints using either array or object syntax, as demonstrated by the storybook story and tests.

These helpers are brought in under the unstable_* namespace as we experiment to figure out the best component API for these helpers.

For reviewers, the styles and examples come roughly from the Grid elements preview

Changelog

New

  • add components/Layout with unstable_Grid, unstable_Row, and unstable_Column helpers

Changed

Removed

@joshblack joshblack requested a review from a team September 2, 2019 22:44
@ghost ghost requested review from alisonjoseph and emyarod and removed request for a team September 2, 2019 22:44
@joshblack
Copy link
Contributor Author

cc @jendowns @SimonFinney do you all have any feedback from the Security side of things for these kinds of helpers? Curious what your thoughts are 😄

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit d4ef1a7

https://deploy-preview-3892--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for carbon-elements ready!

Built with commit d4ef1a7

https://deploy-preview-3892--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit d4ef1a7

https://deploy-preview-3892--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit 806c54f

https://deploy-preview-3892--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for carbon-elements ready!

Built with commit 806c54f

https://deploy-preview-3892--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Sep 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit 806c54f

https://deploy-preview-3892--carbon-components-react.netlify.com

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

do we want to use knobs for the storybook examples? for example we could combine some of the examples, like the array and object syntax examples for specifying offset, or the directional no-gutter examples

<DemoContent>Offset 3</DemoContent>
</Column>
<Column offset={[2]} span={[2]}>
<DemoContent>Offset 3</DemoContent>
Copy link
Member

@emyarod emyarod Sep 3, 2019

Choose a reason for hiding this comment

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

Suggested change
<DemoContent>Offset 3</DemoContent>
<DemoContent>Offset 2</DemoContent>

and same with the other columns below? (match demo content to offset amount)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emyarod I'm definitely torn, when folks are viewing the storybooks often they want something they can copy-paste and knobs can impede that use-case through a layer of indirection.

Copy link
Member

Choose a reason for hiding this comment

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

I definitely don't like knobs when I'm looking to our React examples to try and copy some example code.

Copy link
Member

Choose a reason for hiding this comment

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

the demo content is meant to reflect the offset value though right

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 agreed, I think it's 100% a copy issue

@alisonjoseph
Copy link
Member

Can we offset columns based on breakpoint? Would it be offset={{ sm: 1, md: 1, lg: 1 }} ?

@joshblack
Copy link
Contributor Author

@alisonjoseph yup!

Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

@joshblack this looks awesome! 🎉 So excited for this & I'm sure our application teams would find this useful as well.

I just had 2 general comments.

// Condensed
.example .bx--grid--condensed,
.example .bx--row--condensed {
background-color: #171717;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @joshblack just double-checking if you meant to leave these hex values in here rather than use color tokens? (Not if all of these have ideal theme tokens though)

<Grid>
<Row>
<Column span={[1, 4, 8]}>
<DemoContent>1/4</DemoContent>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at some of these in the preview, it is a little confusing:

Screen Shot 2019-09-03 at 2 17 07 PM

When I first saw how large the first column in the preview was, even though it said 1/4, I wasn't sure what was meant. At first I thought something was wrong until I looked at the values in the span prop.

Is 1/4 meant to illustrate "1 of 4 columns"?
Would possible to put more information about the span inside the DemoContent instead?

@mattalco
Copy link

mattalco commented Sep 4, 2019

@joshblack Do you know if this will eventually support adjustable gutter spacing? Our team has implemented a Grid component based off of Carbon specs but we also added the ability to configured the spacing above/below and between items. This was our reference: https://www.carbondesignsystem.com/guidelines/layout/#gutters

And here's a sample page we have.
grid-gutters

@joshblack
Copy link
Contributor Author

Great question @mattalco, I don't think it will in this initial iteration (default will be to pass a custom className) but in the future I'm hoping we can add <Spacer> components that could be used like:

<Grid>
  <Spacer step={2}>
    <Row>{/* ... */}</Row>
  </Spacer>
</Grid>

@SimonFinney
Copy link
Contributor

@joshblack LGTM! The only thing I'll say is that I feel while the array syntax is good for simplicity and speed, it was more difficult at a glance to grok what was happening in comparison to the object syntax.

@joshblack
Copy link
Contributor Author

Thanks for the feedback all! Going to take everything in and PR a stable release of these components soon 😄

@joshblack joshblack closed this Sep 9, 2019
joshblack added a commit that referenced this pull request Jan 29, 2020
* feat(grid): add hiding columns with width 0

Adds functionality requested in (#2435) with more convenient syntax

* docs(grid): add example of hiding columns

* docs(grid): fix incorrect `bleed` info and typos

* docs(grid): add link to live demo website

Change requested here in (#2803)

* feat(grid): implement functional React component

* chore(grid): add generated files

* fix(docs): storybook support load CSF & add sort

* docs(Grid): update valid props

additionally:
- fix oneOf not using array
- label default props as undefined
- convert col width from str -> num
- add range func

* docs(Grid): add initial story & style margins

* docs(Grid): clean up doc styles & knob controls

* docs(Grid): add color knobs, pass thru params

* docs(Grid): fix minor fullwidth disp bug

* test(Grid): add smoke tests for root el

* test(Grid): add basic row and col tests

* test(Grid): fix wrappers & update index snapshot

* feat(Grid): use layout's breakpoints

* chore(Grid): port all tests, add 'as' prop

* docs(Grid): include stories from #3892

* feat(Grid): support obj syntax & rm offset props

* fix(Grid): add missing @carbon/layout pkg

* feat(Grid): rm `noGutter` prop for later PR

* chore(Grid): revert storybook to 5.2.1

* chore(Grid): convert to recommended syntax

* chore(Grid): inline array gen helper

* fix(Grid): update incorrect prop types

* chore(Grid): update import format

* Revert sass.md

* Revert _mixins.scss

* Revert App.js

* refactor(grid): split out files and update Column

* refactor(grid): update test suite for grid components

* docs(grid): update story for grid

* fix(react): update entrypoint with grid components

* fix(grid): convert story offset to prop obj

* fix(react): update with missing prop values

* chore(project): sync offline mirror

Co-authored-by: Josh Black <josh@josh.black>
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.

6 participants