-
Notifications
You must be signed in to change notification settings - Fork 21
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
Improve and document the TableFilter component #163
Conversation
Adds a notice when no filter options are present Reduces duplicate inputs by utilizing existing input to get necessary information Documents TableFilters and ColumnMenuFilter components
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.
First pass comments ... I may need to try to grok the component actions and reducers to really understand exactly what you've done by removing the filterData
prop...
Adds documentation, and updates the message when no options are present
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.
Addressed review comments. Also, I made improvements to MultiSelectBox.
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.
But I think Varshini's comments are worth addressing.
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.
LGTM!
@@ -3,6 +3,7 @@ import "react-calendar/dist/Calendar.css"; | |||
import "./index.less"; | |||
|
|||
import { | |||
Banner, |
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.
You added the Banner
import, but I don't see it being used. Ah, well. I see you merged, so someone can clean it up later. I'll bet ESLint
will catch it.
I found the TableFilter component confusing due to the duplicate input and lack of documentation. This PR aims to fix that.
This was entirely undocumented before, so let me know if I have any documentation errors.
Type of change
Description
Related Tickets & Documents
Checklist before requesting a review
Testing