keyboard navigation for cluster badge appearance modal #13021
Conversation
… to get focus trap to work properly
…rol + fix element for config badge button
aalves08
force-pushed
the
12781-cluster-badge-config-key-nav
branch
from
e671c29 to
f1f73c5
Compare
…tep, which imo doesnt serve any purpose whatsoever
aalves08
changed the title
| @@ -90,6 +90,7 @@ | |||
| "express": "4.17.1", | |||
| "file-saver": "2.0.2", | |||
| "floating-vue": "5.2.2", | |||
| "focus-trap": "^7.6.2", | |||
There was a problem hiding this comment.
Can we pin focus-trap to the version being used?
| @@ -90,6 +90,7 @@ | |||
| "express": "4.17.1", | |||
| "file-saver": "2.0.2", | |||
| "floating-vue": "5.2.2", | |||
| "focus-trap": "^7.6.4", | |||
There was a problem hiding this comment.
Same here - pin focus-trap to the version that we want to target.
| @@ -16,26 +16,31 @@ export default { | |||
| } | |||
| }, | |||
| methods: { | |||
| customBadgeDialog() { | |||
| customBadgeDialog(ev) { | |||
There was a problem hiding this comment.
ev looks like an unused parameter that can be omitted.
| @click="customBadgeDialog" | ||
| @keyup.space="customBadgeDialog" |
There was a problem hiding this comment.
Does a button require explicitly listening for space? iirc space/enter should trigger a click event in buttons.
| <a | ||
| role="button" | ||
| :aria-label="displayProvider" | ||
| @click="goToHarvesterCluster" | ||
| @keyup.space="goToHarvesterCluster" | ||
| @keyup.enter="goToHarvesterCluster" | ||
| > |
There was a problem hiding this comment.
Do we get any benefit out of preserving the anchor element? I still think that it makes more sense to use a button in these cases, we can use <button class="btn role-link" and that should produce a similar effect.
| # this is the "same" as doing a yarn dev (in a build sense) | ||
| # it's to make sure the dev environment is running properly |
There was a problem hiding this comment.
Is this comment meant for a different PR?
| data() { | ||
| return { focusTrapInstance: {} as FocusTrap }; | ||
| }, | ||
| mounted() { | ||
| if (this.triggerFocusTrap) { | ||
| this.focusTrapInstance = createFocusTrap(this.$refs.cardContainer as HTMLElement, { | ||
| escapeDeactivates: true, | ||
| allowOutsideClick: true, | ||
| }); | ||
|
|
||
| this.$nextTick(() => { | ||
| this.focusTrapInstance.activate(); | ||
| }); | ||
| } | ||
| }, | ||
| beforeUnmount() { | ||
| if (this.focusTrapInstance && this.triggerFocusTrap) { | ||
| this.focusTrapInstance.deactivate(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
I have a hunch that this is something that we can reuse across different components. Have we considered if this would fit into a composable?
Summary
Fixes #12781
Occurred changes and/or fixed issues
cluster explorerviewcluster appearancemodalfocus-trapdirectivecardcomponent to have a focus-trap trigger, controlled via prop, so that it applies a focus-trap to be used in modalstooltipbrowsable/triggerable via keyboard navinput type=colorinputTechnical notes summary
appModalbecause of the nature ofteleportmechanismAreas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
Screen.Recording.2025-01-13.at.14.02.38.mov
Checklist