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.

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

feat: add Grid component #442

Merged
merged 1 commit into from
Jan 18, 2019
Merged

feat: add Grid component #442

merged 1 commit into from
Jan 18, 2019

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Jan 16, 2019 •
edited
Loading

Closes #73

Summary

Implements a <Grid> component that accepts all the supported CSS Grid properties.

<Grid> component
2019-01-18 10 47 17

Example for fixed columns
2019-01-18 10 47 49

Example for auto-sizing columns
2019-01-18 10 48 18

Design

NA

Component Checklist

Technical

  • Documentation

    • Has README.md file
    • Explains use-cases
    • Explains props
    • Has story for Storybook

  • Props

    • Uses prop names consistent with existing components
    • Does not use true for default-props
    • Does not accept classNames prop
    • Has default props for labels (where applicable)

  • Design

    • Does not use hard-coded values
    • Uses design-tokens (instead of e.g. color variables)

  • Testing

  • Accessibility (optional)

    • Supports aria attributes

@emmenko emmenko added the 🚧 Status: WIP Work in progress label Jan 16, 2019
@emmenko
Copy link
Member Author

emmenko commented Jan 16, 2019

This component can obviously be extended and made more configurable as we go, for now I would consider this the bare minimum to make it usable.

tdeekens
tdeekens reviewed Jan 16, 2019
View reviewed changes
src/components/grid/grid.js Outdated
grid-gap: ${customProperties['--spacing-16']};
grid-auto-columns: 1fr;
grid-template-columns: ${props => {
switch (props.mode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if a Grid.Fixed and Grid.AutoFit would be nicer than a prop. It feels a bit like a type on a button. I know it would likely complicate the implementation a bit cause we'd have the GridContainer and compose it with more specific grid-template-columns depending if it's Grid.Fixed and Grid.Autofit. Not sure if the suggestion makes sense or if others would agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can do that. It would avoid "throwing" errors because of unsupported modes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also make one mode the default instead of throwing.

Copy link
Member Author

Choose a reason for hiding this comment

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

f9ac92b

dferber90
dferber90 reviewed Jan 16, 2019
View reviewed changes
src/components/grid/grid.js Outdated
1: '600px',
2: '450px',
3: '350px',
4: '275px',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be the same as our horizontal constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm good point. I can check in the places where we use this already if the sizes make sense.

src/components/grid/grid.js Outdated
1: '600px',
2: '450px',
3: '350px',
4: '275px',
Copy link
Contributor

Choose a reason for hiding this comment

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

Our possible values are all over the place right now. We sometimes use numbers, words or shirt sizes.

I'd use shirt-sizes for now, so s, m, l, xl

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's one of the things we need to check with the design team.

src/components/grid/grid.js
</GridContainer>
);
Grid.displayName = 'Grid';
Grid.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about the API. The current one is pretty restrictive and it would suck if we had to upgrade the whole MC while we learn more about how to provide a flexible API. Exploring more possible use-cases now can probably avoid migration pain afterwards.

Maybe we can explore using Grid for more advanced use-cases before publishing it?

For example, we could try to build the CollapsiblePanel header (if that's even something we intend it to be used for).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we don't need to merge this in right away. We can keep it open for now.

The current one is pretty restrictive

Definitely. There is a lot of room for improvements, this one is the bare minimum.

@emmenko
Copy link
Member Author

emmenko commented Jan 17, 2019

Alright, after talking to @dferber90 we decided to have a low-level <Grid> component that accepts ALL css-grid properties (actually, any css property, we could restrict that if necessary).
This component can be used directly but it should be used as a building block for higher-level grid components that comes with a pre-defined setup: <GridFixed> and <GridAutoFit>.

f9ac92b

@qmateub
Copy link
Contributor

qmateub commented Jan 17, 2019

Nice! Looking forward to use this! Thanks a lot @emmenko 🎉

@emmenko
feat: add Grid component
8832b5d 
feat(grid): add support for data-attributes

refactor(grid): to export Grid.Fixed and Grid.AutoFit components

refactor(grid): to export low-level component Grid and ready-to-use components GridFixed and GridAutoFit

feat(grid): implement css-grid component
@emmenko
Copy link
Member Author

emmenko commented Jan 18, 2019

Alright, I've rewrote the <Grid> component after talking to @dferber90. The component does not do anything per-se now, it just forwards the given css grid props to the styled component.

There are a couple of examples in the storybook to show the setup using fixed columns or auto-sizing columns.

@dferber90
Copy link
Contributor

I approved the new snaps as they're not really design-related ✅

dferber90
dferber90 approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@dferber90 dferber90 left a comment

Choose a reason for hiding this comment

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

🔢

@emmenko emmenko merged commit 7469bae into master Jan 18, 2019
@emmenko emmenko deleted the nm-grid branch January 18, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dferber90 dferber90 dferber90 approved these changes

@tdeekens tdeekens tdeekens approved these changes

Assignees
No one assigned
Labels
🚀 Type: New Feature Something new
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Grid
4 participants
@emmenko @qmateub @dferber90 @tdeekens