Keyboard nav for extensions main screen #13176
Conversation
aalves08
force-pushed
the
12785-key-nav-extensions
branch
3 times, most recently
from
cbb6400 to
3d08265
Compare
aalves08
force-pushed
the
12785-key-nav-extensions
branch
from
491259a to
da77f27
Compare
aalves08
changed the title
| created() { | ||
| if (this.triggerFocusTrap) { | ||
| watcherBasedSetupFocusTrap(() => this.modalVisibility, '.modal-container'); | ||
| } | ||
| }, |
There was a problem hiding this comment.
In pkg/rancher-components/src/components/Card/Card.vue, we do the following
setup(props) {
if (props.triggerFocusTrap) {
basicSetupFocusTrap('#focus-trap-card-container');
}
Is there any reason why we can't do the same here? It looks like we would be able to drop the modalVisibility prop if we were to achieve this.
| @@ -1,8 +1,9 @@ | |||
| <script lang="ts"> | |||
| import { defineComponent, PropType } from 'vue'; | |||
| import { createFocusTrap, FocusTrap } from 'focus-trap'; | |||
| import { basicSetupFocusTrap } from '@shell/composables/focusTrap'; | |||
There was a problem hiding this comment.
It's a common Vue convention for composable functions to start with use. I think that useFocusTrap would be a more idiomatic name for this function.
| }); | ||
| </script> | ||
|
|
||
| <template> | ||
| <div | ||
| ref="cardContainer" | ||
| id="focus-trap-card-container" |
There was a problem hiding this comment.
Is there any reason we can't stick with the ref? I think that it is preferred over an id when working with component instances.
| /** | ||
| * Modal visibility (used for focus-trap) | ||
| */ | ||
| modalVisibility: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, |
There was a problem hiding this comment.
The modalVisibility looks redundant to me.. Shouldn't modal instances be conditionally rendered already?
| import { createFocusTrap, FocusTrap } from 'focus-trap'; | ||
|
|
||
| export function basicSetupFocusTrap(containerSelector: string) { | ||
| let focusTrapInstance = {} as FocusTrap; |
There was a problem hiding this comment.
I think that we can just specify the type:
| let focusTrapInstance = {} as FocusTrap; | |
| let focusTrapInstance: FocusTrap; |
Summary
Fixes #12785
Occurred changes and/or fixed issues
LabeledInputaccessibilityTechnical notes summary
Areas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
Checklist