-
Notifications
You must be signed in to change notification settings - Fork 266
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
Create accessible dropdown component #13019
base: master
Are you sure you want to change the base?
Create accessible dropdown component #13019
Conversation
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.
General comment:
Kind of hard to read for me since I am not familiar with the composition API. Maybe you can guide me today through the PR on our sync and tutor me on some of the composition API aspects.
If I am not mistaken on my analysis (bear with me):
- why is
RcDropdownCollection
needed instead of just usingRcDropdown
? seems to be adiv
with some props - why does
RcDropdownItem
registers event handlers forkeydown
for each individual item? If we have 15 items, we are registering 15x2=30 events. Can't that be achieved in a broader level with only 2 event handlers in a component likeRcDropdown
? - we should have some props on
RcDropdown
to control certain props of the v-dropdown, if needed. We can add those later as per need basis. Same for the other components likearia-label
in theRcDropdownCollection
, if we are going to keep it.
Nevertheless, the functional seems to be exactly what was needed and behaves well. Just some minor adjustments needed on focus:visible, especially on dark mode where the white border and outlines overlap in a weird way, imo.
939287d
to
0779a57
Compare
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
11f9f46
to
5ebb10d
Compare
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
1c6ebbf
to
77bafd1
Compare
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
77bafd1
to
c5dff2b
Compare
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.
- Generally
- Compared to options api, composition api can get really confusing given the amount of
types
andconsts
that are glued together at different points. In more complex components this could lead to a lot of confusion. Is there any way we can clean these up to be more clear? I’ve made some comments but overall i feel we’re missing something.
- Compared to options api, composition api can get really confusing given the amount of
- Testing
- Has this been tested (even in a hacky way) with SSO enabled?
- UX
- When the hover state of the action menu is visible and then it gains the focus state the box grows in side (gains the border). This feels a bit off putting. Can the focus be included with the hover and be a different colour? Need to discuss with UX team?
- Focus state of user avatar (border not flush with button content) is different from focus state of header action menu (border flush with button content
- Bugs
- After clicking on the action menu the drop down is shown. Pressing tab though doesn’t take focus to the first entry in the drop down. Same issue for user avatar (first entry not selected)
Update
Missed two bugs
Dropdown location doesn't seem correct. Chrome Version 132.0.6834.83 (Official Build) (64-bit)
@@ -32,7 +32,7 @@ export default class PageActionsPo extends ComponentPo { | |||
* @returns {Cypress.Chainable} | |||
*/ | |||
private static pageActionsMenu(): Cypress.Chainable { | |||
return cy.get('body').getId('page-actions-dropdown'); |
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 would be a good time to add a PO for the new component
'@typescript-eslint/no-unused-vars': [ | ||
'warn', | ||
{ | ||
argsIgnorePattern: '^_', |
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.
how come we're adding properties that aren't used?
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.
To satisfy a default context for inject via
export const defaultContext: DropdownContext = {
handleKeydown: () => null,
showMenu: (_show: boolean | null) => null,
registerTrigger: (_triggerRef: RcButtonType | null) => null,
focusFirstElement: () => null,
isMenuOpen: ref(false),
close: () => null,
};
inject<DropdownContext>('dropdownContext') || defaultContext;
This convention is useful in other scenarios like when working with callback functions or array methods that might require certain function arguments that are unused in an implementation.
We can rollback this pattern and look for a different approach if preferred.
:container="'.page-actions'" | ||
> | ||
<i | ||
<rc-dropdown :aria-label="t('nav.actionMenu.label')"> |
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.
it still feels like there's a component missing that wraps the need to bring in each of these components and just accepts some labels and an actions collection.
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'm not dismissing the idea, I just don't think that this is the PR to implement it. All the pieces are in place and I want to ensure that we have a consistent an unified data structure to support it.
const props = defineProps<Props>(); | ||
|
||
const buttonClass = computed(() => { | ||
const activeRole = buttonRoles.find(({ role }) => props[role as keyof Props]); |
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.
these as
s seem like a major breakdown of the benefit of composition api (everything is correctly typed).
it looks like the fix is to type buttonRoles / buttonSizes
const buttonRoles: {
role: keyof Props,
className: string,
}[] = [
{ role: 'primary', className: 'role-primary' },
{ role: 'secondary', className: 'role-secondary' },
{ role: 'tertiary', className: 'role-tertiary' },
{ role: 'link', className: 'role-link' },
];
const buttonSizes: {
size: keyof Props,
className: string,
}[] = [
{ size: 'small', className: 'btn-sm' },
];
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.
Yes, that seems preferable. I also extracted the role props and size props into their own types. This allows us to assert that roles and sizes are properly isolated; we can also specify defineProps
via an intersection type of ButtonRoleProps
and ButtonSizeProps
.
} | ||
}; | ||
|
||
const registerDropdownItems = () => { |
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 file has a lot of consts to mentally wire together. to avoid this, this single use function should be inline above
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 think that this function is self-documenting and moving it into register()
requires more thought to understand what the code block intends to do. I'd like to advocate for keeping this one separate.
I'm also attempting to isolate concerns related to the dropdown context and the dropdown collection into separate composables. I think this does a better job of grouping concerns, but has the tradeoff of moving the logic out into a separate file. I will continue to think and iterate on organizing logic for this component.
pkg/rancher-components/src/components/RcDropdown/RcDropdown.vue
Outdated
Show resolved
Hide resolved
pkg/rancher-components/src/components/RcDropdown/RcDropdown.vue
Outdated
Show resolved
Hide resolved
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
I’m working on organization techniques. I still think some of these patterns can be improved upon.
That’s a good callout - SSO will need to be tested. I can deploy these changes and report any modification required when using github auth.
This is how the menus behave today: We are actively engaging @edenhernandez-suse to develop consistent designs and patterns for dropdown menus throughout Dashboard. rancher/ux#113
This is not a bug and is by design. The design guidlines for the menu pattern, provided by w3, detail specific usage of arrow keys and tab behavior. |
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
The placement has been updated |
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
da2f6a3
to
d8adb6e
Compare
Summary
This creates a new dropdown menu component that can be used to create accessible dropdown menus with built-in keyboard navigation.
Fixes #12771
Fixes #12772
Fixes #12790
Occurred changes and/or fixed issues
Technical notes summary
RcDropdown consists of several, tightly-coupled, composable components that are designed to be accessible and provide keyboard navigation out of the box. My primary motivation for promoting component composition over a props-driven approach was to allow for replacing multiple bespoke dropdown menu implementations without requiring too much refactoring in the process. The two dropdown menus that are replaced in this PR demonstrates this ability by showing how RcDropdown can be used to replace the native html element without requiring much effort with regards to considering component logic or keyboard navigation.
Areas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
dropdown-demo.mp4
Checklist