-
Notifications
You must be signed in to change notification settings - Fork 344
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
Patterns page: Add patterns filter and grid/list view toggles #3043
Conversation
content/patterns/patterns.html
Outdated
}); | ||
} | ||
|
||
function updateView(view) { |
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.
For reviewers, this may be an anti-pattern. I can use the aria.X.utilityMethod()
pattern from filterBySearch.js
if preferred. I took this route to avoid complicating the domain of that file or adding another script for such a small task but I can see how this is divergent
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 acknowledging and calling this out. My immediate reaction would indeed be to keep it in the filterBySearch.js
utility.
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.
Agreed! Addressed in 56cfb28
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.
This is great, really looks good to me! Thanks for tackling this task. I only have a few comments:
- The suggestion for an accessible h2 from this comment and this comment are missing. Could those still be added?
- When doing a search with no valid results, I'd prefer some indication of that be shown, eg. No results found or similar. What do you think? I've attached a screenshot below which just shows a blank area
@mcking65 @a11ydoer could you also review this work on the deploy preview's patterns page? |
Nice call. Didn't notice those comments, too focused on the mockup. Also agree with the need for a "no results" element. First handled by 4b0a408 & 5626c7b and accompanying build repo commits. Second addressed in eaf3f2deaf3f2d and accompanying build repo commits. |
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: New patterns page filter<jugglinmike> github: https://github.com//pull/3043 <jugglinmike> Jem: This is awesome! <jugglinmike> Matt_King: I have a few questions <jugglinmike> Matt_King: It seems like the filter happens dynamically as you type. That how it feels to me as a screen reader user <jugglinmike> Matt_King: I didn't have to press the "submit" button, so I don't know what the purpose of the "submit" button is <jugglinmike> Matt_King: Are there any circumstances where you need that button? <jugglinmike> howard-e: It doesn't seem like there is a need for that button <jugglinmike> Matt_King: I do think that a button labeled "Clear Filter" would be very useful. Without it, in order to see all the patterns again, you have to select all the text and delete it <jugglinmike> arigilmore: Carbon offers a feature like this using an "X" button inside the search bar <jugglinmike> Matt_King: The "clear" button is visually inside the search bar, but not in terms of the DOM, right? <jugglinmike> arigilmore: That's right <arigilmore> https://carbondesignsystem.com/components/search/code <jugglinmike> Matt_King: We should change the placeholder text to read "Filter patterns" <jugglinmike> Jem: Maybe the heading should read "Search patterns" instead of "Filter" <jugglinmike> Matt_King: That may be misleading. Some folks may read the worth "Search" and expect that their query will be evaluated against the entire content of each pattern <jugglinmike> Matt_King: As it stands, the query is only matching against the titles of the patterns that are present on the page <jugglinmike> Matt_King: I think that's more generally recognized as a "filter" operation <jugglinmike> Jem: Okay; could we also change the heading to "Filter patterns" instead of just "Filter"? <jugglinmike> Matt_King: Sure |
Updates based on @isaacdurazo 's mockups from the feedback in the APG Task Force meeting:
|
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.
@stalgiag thanks for addressing the latest Task Force feedback. These changes look good to me!
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.
Works great!! Thank you @stalgiag!
@a11ydoer if you agree it is ready to ship, I will merge. |
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.
@stalgiag thank you for all the work on this! We finally have this long-awaited feature! |
see #2534
This PR adds an input to the patterns index page that filters the patterns as you type. Additionally, a toggle is added that allows users to switch between a list and a grid view for the patterns.
Build repo PR
View an example of this at Build repo's netlify preview provided link.
This WAI Preview link will fail to build until wai-aria-practices#1234 is also merged
WAI Preview Link (Last built on Tue, 02 Jul 2024 22:32:14 GMT).