Changed DataFilters to add layer
#6600
Conversation
| <Box fill align="center" justify="start" pad="large"> | ||
| <Data data={DATA}> | ||
| <DataFilters layer> | ||
| <DataFilter property="location" /> |
There was a problem hiding this comment.
| <DataFilter property="location" /> | |
| <DataFilter property="location" contentProps={{ width: 'medium' }} /> |
We've been defaulting to medium width for formfields.
There was a problem hiding this comment.
Does DataFilter pass FormField props through?
There was a problem hiding this comment.
DataFilter does pass contentProps through.
However, the desired behavior feels like something that should be built in or theme driven so that the caller doesn't need to pass this to every DataFilter. I'm actually wondering if we should build this into the theme at theme.formField.content.width. Thoughts?
There was a problem hiding this comment.
I like that idea, it's a lot of repeated implementation to have to do it each time.
| @@ -99,6 +106,13 @@ export const DataFilters = ({ drop, children, heading, ...rest }) => { | |||
| })} | |||
| </Heading> | |||
| {!controlled && clearControl} | |||
| {layer && ( | |||
There was a problem hiding this comment.
Small comment about the Heading with size="small" above (not changed in this PR but just noticing it) in relation to this being BETA and the upcoming HPE typography changes. We are going to steer away from having to apply size on Heading and just leverage the theme definitions. For this, we should:
- Remove size now in prep for brand updates
- Leave, but note that once the next theme version is released we should remove size="small" from the heading
- Add as theme-able thing (but this is probably a bit overkill / opening an unnecessary can of worms
There was a problem hiding this comment.
I'm assuming there are other places, such as Calendar's Heading that would be affected. So, I'm inclined to be consistent with the current master branch and update them all together. I believe this is option two in the list above.
| </Grommet>, | ||
| ); | ||
|
|
||
| expect(container.firstChild).toMatchSnapshot(); |
There was a problem hiding this comment.
Is this test capturing the layer? or do we have to set it to open to capture the layer in the snapshot?
What does this PR do?
Changed DataFilters to add
layer.Where should the reviewer start?
DataFilters.js
What testing has been done on this PR?
Added a story and a unit test
How should this be manually tested?
new story
Do Jest tests follow these best practices?
no change to unit test structure
Any background context you want to provide?
What are the relevant issues?
#6598
Screenshots (if appropriate)
Do the grommet docs need to be updated?
yes
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible