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
Add Filtering Structure for Data Table #1436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks close!
Design notes: A consistent padding all the way around will make it look a bit more polished.
ui/components/FilterBar.tsx
Outdated
} | ||
|
||
const FilterBar = styled(UnstyledFilterBar).attrs({ | ||
className: UnstyledFilterBar.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is not going to make sense as a className
. Can I recommend we do this?:
export default styled(FilterBar).attrs({ className: FilterBar.name })``;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ONLY reason I'm doing it like that is for Storybook to work correctly and automatically. It's definitely ugly and I hate it but I haven't found a way for Storybook to import things automatically otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK lets change the attrs.className
to be a semantic name, ie FilterBar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work on the tests. No blockers 👍
ui/components/FilterBar.tsx
Outdated
const [anchorEl, setAnchorEl] = React.useState(null); | ||
const [showFilters, setShowFilters] = React.useState(false); | ||
|
||
const onCheck = (e, option) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get some type annotations here? What will e
and option
look like when they get passed in here?
case IconType.SearchIcon: | ||
return SearchIcon; | ||
|
||
case IconType.ClearIcon: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge conflict incoming from this PR: #1451
); | ||
expect(screen.queryByText("app")).toBeTruthy(); | ||
expect(screen.queryByText("app3")).toBeTruthy(); | ||
expect(screen.queryByText("Clear All")).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* added outside parts, now working on inside parts * filter bar v1 * updated tests * hitting pause on this awaiting design changes * separated out chips into chip group, addressed pr comments, changing styling to new design * styles and tests * readded icons
* added outside parts, now working on inside parts * filter bar v1 * updated tests * hitting pause on this awaiting design changes * separated out chips into chip group, addressed pr comments, changing styling to new design * styles and tests * readded icons
Sets up a filtering component to be used in parent component of
<DataTable />
- functionality will be passed down. I'm fully expecting to have to add some styling polish. so hit me with what needs to change.Check it out by pulling down the
filter-bar
branch and runningnpm run storybook