-
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
US11 // parts - highlight active filters #30
Conversation
…so for statusFilter
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Nice work! 🚀
See some tiny suggestions on naming.
Cheers,
Thomas
components/PartsList/index.js
Outdated
]} | ||
activeFilter={activeCategoryFilter} | ||
/> | ||
<StyledFilter | ||
sets={[ |
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.
A smart idea to populate your filters! 🚀
I suggest some naming changes to shed some light on it for external reviewers (and for yourself in the future when you review your code again after some time has passed 😉 )
{
primaryFunction: setStatusFilter,
primaryValue: "alle",
filterName: "alle",
secondaryFunction: setActiveStatusFilter,
secondaryValue: "alle",
}
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.
Thanks for your advise. I will stay consistent in naming from now on ✌🏼
it looks much better now
// reworked naming |
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 now 🚀
highlighted the active filter for each of the filter-slider
US11
Deployment
click a filter and see the highlighting. (just a little bit)