feat: Update Grid components to render any type of element #1166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…date tests, update storybook with customContainer story
SirenaBorracha
requested review from
ahobson,
brandonlenz,
gidjin,
haworku,
sarboc,
suzubara and
tinyels
as code owners
brandonlenz
reviewed
Apr 27, 2021
src/components/grid/Grid.stories.tsx
Outdated
Comment on lines
66
to
71
| <li> | ||
| <Grid row> | ||
| <Grid col={1}>{testContent}</Grid> | ||
| <Grid col={11}>{testContent}</Grid> | ||
| </Grid> | ||
| </li> |
There was a problem hiding this comment.
I have some thoughts on this, but they're kinda hard to articulate, because the use case for doing this is still a bit weird to me.
There's nothing inherently wrong with putting Grid components inside an li in this case, but it is basically a perfect opportunity to use the custom Grid component as a li anyways.
I see you have a story for the custom Grid item as well, so you have that covered. My thinking at least is to just combine the two into a single customElements story where both the container and the grid items are custom elements (container can be ul and grid can be li maybe?)
There was a problem hiding this comment.
I love this idea!
brandonlenz
reviewed
Apr 27, 2021
src/components/grid/Grid.stories.tsx
Outdated
Comment on lines
66
to
71
| <li> | ||
| <Grid<CustomGridProps> asCustom={CustomGrid}> | ||
| <Grid col={11}>{testContent}</Grid> | ||
| <Grid col={2}>{testContent}</Grid> | ||
| </Grid> | ||
| </li> |
There was a problem hiding this comment.
Now that you've updated your custom grid to be a li, this is going to render li nested within a li. Does it work if you get rid of the parent li tags like so?:
Suggested change
| <li> | |
| <Grid<CustomGridProps> asCustom={CustomGrid}> | |
| <Grid col={11}>{testContent}</Grid> | |
| <Grid col={2}>{testContent}</Grid> | |
| </Grid> | |
| </li> | |
| <Grid<CustomGridProps> asCustom={CustomGrid}> | |
| <Grid col={11}>{testContent}</Grid> | |
| <Grid col={2}>{testContent}</Grid> | |
| </Grid> |
…ult from custom component creation
* Update isCustomProps to accept "otherProps" * Refactor out ommited props
brandonlenz
approved these changes
Apr 29, 2021
SirenaBorracha
deleted the
ak-update-grid-component-to-take-component-prop-160
branch
Merged
SirenaBorracha
added a commit
that referenced
this pull request
May 5, 2021
## [1.17.0](1.16.0...1.17.0) (2021-05-05) ### Features * Checkbox Tile Variant ([#1104](#1104)) ([9936c4a](9936c4a)) * Implement ProcessListHeading subcomponent ([#1162](#1162)) ([964e34c](964e34c)) * New Component ProcessList MVP ([#1107](#1107)) ([1bc0f93](1bc0f93)) * New Component SiteAlert ([#1099](#1099)) ([c1e88e0](c1e88e0)) * New Component SummaryBox ([#1098](#1098)) ([b2279b4](b2279b4)) * Radio Button Tile Variant ([#1101](#1101)) ([a2f40a0](a2f40a0)) * Update Grid components to render any type of element ([#1166](#1166)) ([07468c8](07468c8)), closes [#1194](#1194) * Update Search component to support i18n ([#1192](#1192)) ([5241d15](5241d15)) * Update Table to 2.10.0 implementation ([#1110](#1110)) ([117a6c7](117a6c7))
brandonlenz
added a commit
that referenced
this pull request
May 12, 2021
* Update GridContainer to handle any component but default to a div, update tests, update storybook with customContainer story * Combine GridContainer examples in storybook * Update Grid component to render any type of element * Regenerate yarn.lock * Combine custom components into one Storybook example * Remove superfluous li element from custom storybook example * Remove unnecessary spaces from storybook file * Move call to classes function outside of if statement separating default from custom component creation * Update isCustomProps to accept "otherProps" (#1194) * Update isCustomProps to accept "otherProps" * Refactor out ommited props Co-authored-by: Brandon Lenz <brandonalenz@gmail.com>
brandonlenz
pushed a commit
that referenced
this pull request
May 12, 2021
## [1.17.0](1.16.0...1.17.0) (2021-05-05) ### Features * Checkbox Tile Variant ([#1104](#1104)) ([9936c4a](9936c4a)) * Implement ProcessListHeading subcomponent ([#1162](#1162)) ([964e34c](964e34c)) * New Component ProcessList MVP ([#1107](#1107)) ([1bc0f93](1bc0f93)) * New Component SiteAlert ([#1099](#1099)) ([c1e88e0](c1e88e0)) * New Component SummaryBox ([#1098](#1098)) ([b2279b4](b2279b4)) * Radio Button Tile Variant ([#1101](#1101)) ([a2f40a0](a2f40a0)) * Update Grid components to render any type of element ([#1166](#1166)) ([07468c8](07468c8)), closes [#1194](#1194) * Update Search component to support i18n ([#1192](#1192)) ([5241d15](5241d15)) * Update Table to 2.10.0 implementation ([#1110](#1110)) ([117a6c7](117a6c7))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors
GridandGridContainerto be able to render any type of element with their default as adivRelated Issues or PRs
closes #160
How To Test
Check out this branch and run
yarn testto execute tests andyarn storybookto see the component in action.Alternatively, you can scroll to the bottom of this page and click "Show environments" on the right above the comment input box to access this component in Storybook.
Screenshots (optional)
Nothing is changed visually in this PR from the existing Grid components.