-
Notifications
You must be signed in to change notification settings - Fork 0
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
[BB-9101] Add missing default filter #65
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## asu-moe/redwood-css #65 +/- ##
=======================================================
- Coverage 92.54% 92.48% -0.06%
=======================================================
Files 706 706
Lines 12515 12529 +14
Branches 2728 2737 +9
=======================================================
+ Hits 11582 11588 +6
- Misses 898 906 +8
Partials 35 35 ☔ View full report in Codecov by Sentry. |
051a2d7
to
9ceb989
Compare
3bb1e37
to
fa800c9
Compare
if (menuType !== 'courseTypes') { | ||
return; | ||
} |
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.
@pkulkark Why do we only make this behavior work for one type of the menu? Shouldn't default value work correctly for all menu types?
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.
@Cup0fCoffee It does, but since the same filter component is used for two different menus rendered one after another, the second one resets the first. And since we don't need to change the default behaviour of the second filter, I decided this option would be better. What do you think?
@@ -26,6 +27,13 @@ const CoursesFilterMenu = ({ | |||
if (cleanFilters) { | |||
setItemMenuSelected(defaultItemSelectedText); |
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.
@pkulkark Don't you just need to call the onItemMenuSelected
here? Or even better:
setItemMenuSelected(defaultItemSelectedText); | |
const defaultItemSelectedValue = menuItems.find(item => item.name === defaultItemSelectedText); | |
handleCourseTypeSelected(defaultItemSelectedText, defaultItemSelectedValue); |
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.
@Cup0fCoffee I actually couldn't fully figure out how the cleanFilters
option could be updated, so I didn't change the existing behaviour.
Closing this in favor of #66 |
This PR adds the missing line from #63 which sets the default filter to active courses.