-
Notifications
You must be signed in to change notification settings - Fork 89
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
Library Home - Filter by Tags #1048
Comments
@ChrisChV I think this should actually be using |
@ChrisChV For your other questions, it should work the same way as the course search modal that we built. |
@bradenmacdonald Thanks for the info! |
@bradenmacdonald Could you add this to the milestone? |
This functionality is already implemented for the course search modal. You can re-use the same components, so you don't have to re-implement this.
Don't re-use the code - just use |
@ChrisChV @bradenmacdonald There seems to be 2 styles in the Figma for how the filters should look like: Style 1: Style 2: They are next to each other in Figma, however I'm not sure which one is the final one we are going with? |
@yusuf-musleh This is the Hifi Header |
@yusuf-musleh @ChrisChV The filter components already exist - just re-use the existing ones that we have. They look more like "Style 1". I think they don't have icons though, so adding icons would be a nice addition, but not necessary for the MVP. |
@bradenmacdonald @ChrisChV Got it, thanks for the clarification. Yes we're using the existing components, i've added the icons as well, in this PR. |
This seems to work as intended, but it's a little hard to test without much content in the library. I'm going to suggest a new label "needs to be re-tested" so that we can come back to these again later. |
@jmakowski1123 Can I mark this as Done in the meantime, or do you want me to leave it open in "Ready for AC testing"? |
In the current staging build, the click targets are a bit strange for filter and sort #1038. The full area highlighted gray should be a click target for the item: |
Once Sam's comment is addressed, we can move this to done, then circle back to all the done status later for re-testing. |
Hi @sdaitzman! CC @jmakowski1123 |
Looks good to me, thanks for implementing the change! |
Mockups: Figma mid-fi
Clear filter
button removes the selected tags.The text was updated successfully, but these errors were encountered: