-
Notifications
You must be signed in to change notification settings - Fork 25
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
2704 add reset button to gallery page #3350
Conversation
…Webpage into 2704-add-reset-button-to-gallery-page
@tanayaxsharma this is what it looks like for me, the clear filters bit isn't showing up :'( I emptied cache and did a hard reload to see if it was something cached, but nothing changed. I'm using Google Chrome. But I did just pull in changes from develop, so it's possible that that could have caused an issue..? Could you test again and see if you see the same thing? |
So sorry, "label type" wasn't classified as a filter, which is why I wasn't seeing it. OOPS. |
Makes me wonder if the UI should be rearranged so that label type is listed as a filter. Maybe being able to select multiple label types..? Thoughts for the future! Will finish the review later! |
Yes. I think it is a filter!
On Thu, Aug 17, 2023 at 3:25 PM Michael Saugstad ***@***.***> wrote:
Makes me wonder if the UI should be rearranged so that label type is
listed as a filter. Maybe being able to select multiple label types..?
Thoughts for the future! Will finish the review later!
—
Reply to this email directly, view it on GitHub
<#3350 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAML55KULNEE3UZFSWMR7C3XV2K65ANCNFSM6AAAAAA3MFPJ7E>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
Sent from phone
|
@jonfroehlich thoughts on just removing the "Show" section, and moving the label type filter as the first thing in the "filter by" section? I figure that we can improve on that design going forward, but that might be good enough for now? |
Sounds good!
|
Cool. @tanayaxsharma can you actually do this and then include the label type as part of the "reset filters" button?
|
…Webpage into 2704-add-reset-button-to-gallery-page
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.
I don't think that I like the horizontal bar above "Reset All", and I especially don't like it when there's nothing to reset.
I think I like it better without the line, maybe 5 px of margin above rather than 10 px, and I think we can change from "Reset All" to "Clear Filters". In #2704 we discussed using "Reset All" because label type wasn't being considered a filter, which is no longer an issue :)
Other than that, I added a few comments throughout the code, but I think that they are all pretty minor changes!
I made updates to my code as per your comments! @misaugstad |
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.
Looks great!
Resolves #2704
Added a reset button to the gallery page that clears all filters by using the "unapply" methods from the SeverityBucket.js and ValidationBucket.js files and by implementing setToDefault() methods for ValidationBucket.js and labelTypeMenu.js.
Before/After screenshots
Before:
After:
(before resetting filters)
(after resetting filters)
When testing this, if you originally selected by a certain label type, the cards won't always visibly appear to have reset after clicking the button. However, if you flip through the pages, you will see that all label types are included.