Conversation
|
github-actions
bot
added
work in progress
✅ Deploy Preview for patternfly-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
PTAL at the prior art in rh-pagination, and see how much you can move to shared code like a controller or some such |
|
@bennypowers yep, I'll compare the two implementations now. |
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
Overall a good draft. I have a few suggestions marked.
(And it gave me an opportunity to really, really have to think about one of those tricker accessibility questions, so thanks much for helping me get better at my work!)
| class="${classMap({ expanded: this._expanded })}"> | ||
| <pf-button id="menu-toggle" | ||
| aria-expanded=${this._expanded} | ||
| aria-haspopup="listbox" |
There was a problem hiding this comment.
The aria-attributes you have done correctly correct--nice!
(Note: There's this subtle distinction between a combobox with listbox and a menu, that even I had to really pause and think on. And I almost wanted to change your aria-haspopup to menu above, but then I realized that you are correct and this is really doing what a <select> would do and retains your selection, not what a menu would do. This choice will be important in my comments later because it will determine other choices, and my comments, in the code.)
You will also need to add the following:
-
aria-controls="menu-list" -
role="combobox" -
aria-label="Show items"(or whatever label makes sense) -
aria-activedescendantwhen listbox is open, this should be set toidof listbox option receiving focus
| <span><b>${this.#firstOfPage()}-${this.#lastOfPage()}</b> ${TITLES.ofWord} <b>${this.count}</b></span> | ||
| </pf-button> | ||
| <ul id="menu-list" | ||
| part="menu-list"> |
There was a problem hiding this comment.
add role="listbox" since we're going with listbox over menu.
| icon=${ifDefined(this.#selected(option) ? 'check' : undefined)} | ||
| icon-position=${ifDefined(this.#selected(option) ? 'right' : undefined)} | ||
| part="menu-item" | ||
| role="menuitem"> |
There was a problem hiding this comment.
change to role=option since we're going with listbox
| part="menu-toggle" | ||
| plain | ||
| type="button" | ||
| @click=${this.#toggleExpanded}> |
There was a problem hiding this comment.
You'll also need to add @keydown to handle the following:
- Down Arrow : Opens the listbox if it is not already displayed without moving focus or changing selection. DOM focus remains on the combobox.
- Alt + Down Arrow: Opens the listbox without moving focus or changing selection.
- Up Arrow: First opens the listbox if it is not already displayed and then moves visual focus to the first option. DOM focus remains on the combobox.
- Home: Opens the listbox and moves visual focus to the first option.
- End: Opens the listbox and moves visual focus to the last option.
- Printable characters: First opens the listbox if it is not already displayed and then moves visual focus to the first option that matches the typed character. If multiple keys are typed in quick succession, visual focus moves to the first option that matches the full string. If the same character is typed in succession, visual focus cycles among the options starting with that character. <--- I feel like in this use case, it's a nice-to-have but doesn't have to be part of an MVP.
See Closed combobox
| ${PER_PAGE_OPTIONS.map(option => html`<li role="none"> | ||
| <pf-button variant="tertiary" | ||
| block | ||
| class="${classMap({ ['menu-item']: true, selected: this.#selected(option) })}" |
There was a problem hiding this comment.
you'll also want to set an aria-selected based on which item is currently selected
| this.#paginate('per-page'); | ||
| } | ||
|
|
||
| @bound private _outsideClick(event: MouseEvent) { |
There was a problem hiding this comment.
Another way to do this might be to capture @focusout on your combo/listbox.
| } | ||
|
|
||
| #toggleExpanded() { | ||
| this._expanded = !this._expanded; |
There was a problem hiding this comment.
When listbox closes, focus needs to go back to the toggle button.
| plain | ||
| type="button" | ||
| @click=${this.#toggleExpanded}> | ||
| <span><b>${this.#firstOfPage()}-${this.#lastOfPage()}</b> ${TITLES.ofWord} <b>${this.count}</b></span> |
There was a problem hiding this comment.
${this.count} doesn't seem to be working in demo.
| return; | ||
| case Action.PerPage: | ||
| if (value) { | ||
| this.perPage = parseInt(value, 10); |
There was a problem hiding this comment.
As a user:
- If I'm viewing 10 items at a time, and I'm on page 4 (items 31-40), when I switch to 20 per page, I'd expect to be on page 2 (items 21-40) instead of page 4 (items 61-80) so that I wouldn't miss items.
- If I'm viewing 20 items at a time, and I'm on page 4 (items 61-80), when I switch to 10 per page, I'd expect to be on page 6 (items 61-70) instead of page 4 (items 31-40) so that I wouldn't have to navigate back to the items I was on.
elements/pf-button/pf-button.css
Outdated
| :host([block]) button { | ||
| display: flex; | ||
| width: 100%; | ||
| justify-content: var(--pf-c-button--JustifyContent, center); |
There was a problem hiding this comment.
I removed this and opened that separate PR to add the CSS var, but opting for a part on the button instead.
https://github.com/patternfly/patternfly-elements/pull/2470/files
4 participants
What I did
Testing Instructions
Notes to Reviewers