Grid component react #4894

zachhardesty7 · 2019-12-17T00:35:49Z

This is my first major PR (for Carbon & in general) so any feedback is appreciated! My code focuses heavily on readability and simplicity while mimicking design patterns from other React components, Carbon and otherwise.

Add layout helpers for managing a Grid alongside the Carbon Design System layout guidance. Namely, this PR adds support for Grid, Row, and Col 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 individual props, as demonstrated by the storybook story and tests. (mostly copied description)

All of this work started before @joshblack's PR #3892. I should have opened an issue sooner... I've mostly incorporated all of his PR, but notable differences of this PR include:

noGutterLeft & noGutterRight merged into noGutter as "left" and "right"
no object syntax, all props separate. this decision was made to mimic some very popular libraries (Material UI, Ant Design, React Bootstrap). I do also like the object syntax so that could be merged.
no array syntax ^, but makes helper functions simpler & cleaner
calls classnames function with params & avoids object key calcs for simplicity
a lot of focus on code readability
propTypes defines supported column span as specific numbers
Grid supports noGutter
all components don't define any variables in function body
includes all of the same tests plus a few extra and they all use shallow rendering
includes dynamic knob-based stories while also preserving the easy to copy and paste ones. maybe too many stories?

Changelog

New

add Grid component with supporting Grid.Row (and GridRow) and Grid.Col (and GridCol)

Changed

minor Storybook configs to support new CSF

Removed

Testing / Reviewing

The styles and examples come roughly from the Grid elements preview.

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

Change requested here in (#2803)

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

netlify · 2019-12-17T00:46:25Z

Deploy preview for the-carbon-components ready!

Built with commit 34c2cbf

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

netlify · 2019-12-17T00:49:49Z

Deploy preview for carbon-elements ready!

Built with commit 34c2cbf

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

netlify · 2019-12-17T00:50:53Z

Deploy preview for carbon-components-react failed.

Built with commit 34c2cbf

https://app.netlify.com/sites/carbon-components-react/deploys/5e31ce0762b86e0008b279b9

joshblack · 2019-12-17T23:39:27Z

Hi @zachhardesty7! 👋 Thanks so much for taking the time to put this together! I will definitely take a look at this tomorrow!! 🥳

joshblack · 2019-12-18T18:29:14Z

Taking a quick look, any chance we could remove support for condensed and noGutter? There might be changes in terminology for these so they might not be settled enough to go into stable.

Definitely understand the prop changes in part, I think span and offset were patterns brought over from styled system-esque APIs in my earlier PR. With this named approach, I think something that can feel awkward are the offset* props that come up, as a result (offsetSm, offsetMd, etc). It seems like React bootstrap handles these the best but even within their model one can't do a shorthand notation for offsets which is a bummer.

Are there any other examples that you'd recommend looking at for an ideal grid API here? Seems like we would want:

Auto column support
Span n columns per breakpoint
Offset n columns per breakpoint

zachhardesty7 · 2019-12-22T01:28:08Z

@joshblack

I only implemented those since the Grid component/element package demo and the outputted CSS support them. Does the doc here (the red X) indicate that the classes output by the SCSS shouldn't be used? It wasn't quite clear to me whether they're internal or not. It's an easy change though!

I'm not a huge fan of the offset stuff as separate props either, feels a bit messy. I suppose it could be done via a single offset prop in object notation: offset={ sm: 1, lg: 10, max: 12 }. My concern there was not having symmetry with the span props (sm, md). And then should there be two separate ways for the span props via object notation (eg span={ sm: 1, lg: 10, max: 12 } to maintain symmetry?

This implementation handles auto via the string "auto". Bootstrap supports that and a value of true.

In personal and freelance projects, it feels like I've rarely used props as a part of the Grid, but maybe it would benefit us to have some numbers behind how often props are used? I haven't done anything with parsing Github code usage, but I suspect the most popular APIs should mirror usage. Let me look into it a bit further and respond with some more detailed resources of what I find!

zachhardesty7 · 2019-12-24T22:39:57Z

@joshblack
Okay, so {breakpoint}Offset style props are not good. The better question is what way "offset" should be handled. Here are what some example React libraries do:

Bootstrap React/Reactstrap - no offset prop, must use obj syntax for each breakpoint prop (sm, md)
Antd - offset prop applies to all screen sizes, obj syntax for breakpoint specific offset
MD - no offset prop
SemanticUI - no offset prop

Antd has a cool way of doing it, but since they base it on the Bootstrap style anyway, it makes the most sense to get rid of the offset prop entirely and just support sm={4}, sm={true || false || "auto"} and also lg={{ span: 6, offset: 2 }}. Shouldn't take long to incorporate that here.

Disqualified examples / research data conclusions

(note: repos without a significant commit within about the last year are excluded)

Misc

React MD - too many props
Rebass - CSS-in-JS
Foundation - can't find prop docs?
Pinterest Gestalt - too many props

Seem much more like Carbon DataTable

Grid Layout, AG Grid, Griddle

No flexbox-style responsive grid component (e.g. uses CSS Grid style or no Grid included)

Blueprint, React Toolbox, Belle, Khan Academy, Hedron, Atlas Kit, MS Office Fabric, Grommet

Grid component is not responsive / does not support media queries

Onsen UI, React Virtualized

joshblack · 2020-01-20T23:54:51Z

@zachhardesty7 would you mind if I push some updates to your PR here or would you prefer if I opened up a PR to merge into this branch? I just wanted to double-check with you to see how you felt about the changes, didn't want it to feel like we were disregarding any of your work here as it's all awesome! Thanks again for taking the time to contribute!

zachhardesty7 · 2020-01-21T19:23:36Z

@joshblack Whatever makes the most sense / seems the most reasonable to you is fine by me!

joshblack · 2020-01-22T20:11:03Z

@zachhardesty7 just pushed some updates, would love to know what you think! Tried to do the following:

Split out components into separate files (Grid, Row, and Column)
Moved tests that you originally had over into separate files under __tests__ and updated to use our new test helper
- Tried to use existing tests alongside adding some other checks or doing matching not on exact selector
Updated story to include the templates since they seemed really solid, don't think anything changed there
Updated the Col component and how it derived the relevant class names, let me know if I missed any functionality here!

…grid-component-react

joshblack · 2020-01-22T20:19:17Z

FYI @alisonjoseph too for Grid components 👀 Relevant storybook story for usage: https://github.com/carbon-design-system/carbon/pull/4894/files#diff-d9adc48479ce140b1bc043298685114c

packages/react/src/components/Grid/Grid-story.js

packages/react/src/components/Grid/Column.js

packages/react/src/components/Grid/__tests__/Column-test.js

packages/react/src/components/Grid/Column.js

joshblack · 2020-01-27T23:14:15Z

Updated @zachhardesty7! Thanks again!

…grid-component-react

…n into grid-component-react

abbeyhrt

This looks good to me! Thank you @zachhardesty7 for taking the time to do this! 🎉

zachhardesty7 added 18 commits December 16, 2019 13:56

feat(grid): add hiding columns with width 0

20e4bb3

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

docs(grid): add example of hiding columns

1290f60

docs(grid): fix incorrect bleed info and typos

6a746be

docs(grid): add link to live demo website

031d46a

Change requested here in (#2803)

feat(grid): implement functional React component

cd46e13

chore(grid): add generated files

176d0a9

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

0d8c4ad

docs(Grid): update valid props

203655c

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

e9020ac

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

9de2fd2

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

1c84f6a

docs(Grid): fix minor fullwidth disp bug

ae19981

test(Grid): add smoke tests for root el

af7b92e

test(Grid): add basic row and col tests

fd48342

test(Grid): fix wrappers & update index snapshot

302fd8a

feat(Grid): use layout's breakpoints

a54fc63

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

7e5d938

docs(Grid): include stories from #3892

0a945ff

zachhardesty7 marked this pull request as ready for review December 17, 2019 00:40

zachhardesty7 requested a review from a team as a code owner December 17, 2019 00:40

ghost requested review from aledavila and asudoh December 17, 2019 00:40

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

acd0d48

joshblack added 3 commits January 21, 2020 16:51

refactor(grid): split out files and update Column

0017c8f

refactor(grid): update test suite for grid components

ee23e15

docs(grid): update story for grid

8384714

joshblack added 2 commits January 22, 2020 14:13

Merge branch 'master' of github.com:carbon-design-system/carbon into …

9c65ae4

…grid-component-react

fix(react): update entrypoint with grid components

0833e0b

joshblack requested review from abbeyhrt and tw15egan January 22, 2020 20:18

fix(grid): convert story offset to prop obj

898df9e

zachhardesty7 commented Jan 22, 2020

View reviewed changes

packages/react/src/components/Grid/Grid-story.js Outdated Show resolved Hide resolved

zachhardesty7 commented Jan 22, 2020

View reviewed changes

packages/react/src/components/Grid/Column.js Show resolved Hide resolved

packages/react/src/components/Grid/Column.js Outdated Show resolved Hide resolved

packages/react/src/components/Grid/__tests__/Column-test.js Outdated Show resolved Hide resolved

zachhardesty7 commented Jan 23, 2020

View reviewed changes

packages/react/src/components/Grid/Column.js Show resolved Hide resolved

fix(react): update with missing prop values

520e9aa

joshblack added 6 commits January 27, 2020 17:31

Merge branch 'master' of github.com:carbon-design-system/carbon into …

36b923e

…grid-component-react

Merge branch 'master' into grid-component-react

5bbab2b

chore(project): sync offline mirror

e65f750

Merge branch 'grid-component-react' of github.com:zachhardesty7/carbo…

3efc941

…n into grid-component-react

Merge branch 'master' into grid-component-react

95d3135

Merge branch 'master' into grid-component-react

34c2cbf

abbeyhrt approved these changes Jan 29, 2020

View reviewed changes

aledavila approved these changes Jan 29, 2020

View reviewed changes

joshblack merged commit 354baa8 into carbon-design-system:master Jan 29, 2020

joshblack mentioned this pull request Jan 31, 2020

chore(project): update carbon to v10.10.0-rc.0 #5236

Closed

This was referenced Feb 25, 2020

chore(release): v10.10.0 #5432

Closed

v10.10.0 Release #5398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grid component react #4894

Grid component react #4894

zachhardesty7 commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

joshblack commented Dec 17, 2019

joshblack commented Dec 18, 2019

zachhardesty7 commented Dec 22, 2019

zachhardesty7 commented Dec 24, 2019

joshblack commented Jan 20, 2020

zachhardesty7 commented Jan 21, 2020

joshblack commented Jan 22, 2020

joshblack commented Jan 22, 2020 •

edited

Loading

joshblack commented Jan 27, 2020

abbeyhrt left a comment

Grid component react #4894

Grid component react #4894

Conversation

zachhardesty7 commented Dec 17, 2019 • edited Loading

Changelog

Testing / Reviewing

netlify bot commented Dec 17, 2019 • edited Loading

netlify bot commented Dec 17, 2019 • edited Loading

netlify bot commented Dec 17, 2019 • edited Loading

joshblack commented Dec 17, 2019

joshblack commented Dec 18, 2019

zachhardesty7 commented Dec 22, 2019

zachhardesty7 commented Dec 24, 2019

Disqualified examples / research data conclusions

Misc

Seem much more like Carbon DataTable

No flexbox-style responsive grid component (e.g. uses CSS Grid style or no Grid included)

Grid component is not responsive / does not support media queries

joshblack commented Jan 20, 2020

zachhardesty7 commented Jan 21, 2020

joshblack commented Jan 22, 2020

joshblack commented Jan 22, 2020 • edited Loading

joshblack commented Jan 27, 2020

abbeyhrt left a comment

Choose a reason for hiding this comment

zachhardesty7 commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

netlify bot commented Dec 17, 2019 •

edited

Loading

joshblack commented Jan 22, 2020 •

edited

Loading