-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[VAULT-35469] UI: navigate to namespace on enter/return #30372
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import Component from '@glimmer/component'; | |
| import { action } from '@ember/object'; | ||
| import { tracked } from '@glimmer/tracking'; | ||
| import { service } from '@ember/service'; | ||
| import { KEYS } from 'core/utils/keyboard-keys'; | ||
|
|
||
| /** | ||
| * @module NamespacePicker | ||
|
|
@@ -30,6 +31,7 @@ export default class NamespacePicker extends Component { | |
|
|
||
| @tracked allNamespaces = []; | ||
| @tracked searchInput = ''; | ||
| @tracked searchInputHelpText = 'Enter a full path in the search bar and hit ↵ key to navigate faster.'; | ||
|
||
| @tracked selected = {}; | ||
|
|
||
| constructor() { | ||
|
|
@@ -72,13 +74,17 @@ export default class NamespacePicker extends Component { | |
| ]; | ||
|
|
||
| // Conditionally add the root namespace | ||
| if (this.auth.authData.userRootNamespace === '') { | ||
| if (this.auth?.authData?.userRootNamespace === '') { | ||
| options.unshift({ id: 'root', path: '', label: 'root' }); | ||
| } | ||
|
|
||
| return options; | ||
| } | ||
|
|
||
| get hasSearchInput() { | ||
| return this.searchInput?.trim().length > 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious why we need to trim the input here? Do we expect people will enter a space only?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more just due diligence/habit. I can't tell you how many times I've thought, "a user would never..." only to be immediately proved wrong lol
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Solid call out. And a note for posterity, I checked with the API — we do not allow spaces in namespaces, so there is no chance someone could intentionally want to search a word with a space at the end. |
||
| } | ||
|
|
||
| get namespaceLabel() { | ||
| return this.searchInput === '' ? 'All namespaces' : 'Matching namespaces'; | ||
| } | ||
|
|
@@ -95,11 +101,6 @@ export default class NamespacePicker extends Component { | |
| } | ||
| } | ||
|
|
||
| @action | ||
| onSearchInput(event) { | ||
| this.searchInput = event.target.value; | ||
| } | ||
|
|
||
| @action | ||
| async fetchListCapability() { | ||
| // TODO: Revist. This logic was carried over from previous component implmenetation. | ||
|
|
@@ -115,6 +116,12 @@ export default class NamespacePicker extends Component { | |
| } | ||
| } | ||
|
|
||
| @action | ||
| focusSearchInput(element) { | ||
| // On mount, cursor should default to the search input field | ||
| element.focus(); | ||
| } | ||
|
|
||
| @action | ||
| async loadOptions() { | ||
| // TODO: namespace service's findNamespacesForUser will never throw an error. | ||
|
|
@@ -127,15 +134,34 @@ export default class NamespacePicker extends Component { | |
| await this.fetchListCapability(); | ||
| } | ||
|
|
||
| @action | ||
| async refreshList() { | ||
| this.searchInput = ''; | ||
| await this.loadOptions(); | ||
| } | ||
|
|
||
| @action | ||
| async onChange(selected) { | ||
| this.selected = selected; | ||
| this.router.transitionTo('vault.cluster.dashboard', { queryParams: { namespace: selected.path } }); | ||
| } | ||
|
|
||
| @action | ||
| async onKeyDown(event) { | ||
| if (event.key === KEYS.ENTER && this.searchInput?.trim()) { | ||
| const matchingNamespace = this.allNamespaces.find((ns) => ns.label === this.searchInput.trim()); | ||
|
|
||
| if (matchingNamespace) { | ||
| this.selected = matchingNamespace; | ||
| this.router.transitionTo('vault.cluster.dashboard', { | ||
| queryParams: { namespace: matchingNamespace.path }, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @action | ||
| onSearchInput(event) { | ||
| this.searchInput = event.target.value; | ||
| } | ||
|
|
||
| @action | ||
| async refreshList() { | ||
| this.searchInput = ''; | ||
| await this.loadOptions(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,13 @@ | |
| * SPDX-License-Identifier: BUSL-1.1 | ||
| */ | ||
|
|
||
| /* | ||
| * DEPRCATED (see: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, if we don't already can you make a ticket for this and throw it in our backlog or add to sustaining?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. our copilot prompt caught this one!! 🎉 I'll create a ticket and add it to sustaining |
||
| * | ||
| * TODO: Replace all instances with `event.key` (use lib/core/addon/utils/keyboard-keys.ts). | ||
| * `event.keyCode` is deprecated and will be removed in future versions of browsers. | ||
| */ | ||
|
|
||
| // a map of keyCode for use in keyboard event handlers | ||
| export default { | ||
| ENTER: 13, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /** | ||
| * Copyright (c) HashiCorp, Inc. | ||
| * SPDX-License-Identifier: BUSL-1.1 | ||
| */ | ||
|
|
||
| /** | ||
| * @module keyboard-keys | ||
| * @description Utility for mapping key names to their corresponding `event.key` values for use in keyboard event handlers. | ||
| */ | ||
| export enum KEYS { | ||
| ENTER = 'Enter', | ||
| ESC = 'Escape', | ||
| TAB = 'Tab', | ||
| LEFT = 'ArrowLeft', | ||
| UP = 'ArrowUp', | ||
| RIGHT = 'ArrowRight', | ||
| DOWN = 'ArrowDown', | ||
| T = 'F5', | ||
| BACKSPACE = 'Backspace', | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,17 @@ | |
| * SPDX-License-Identifier: BUSL-1.1 | ||
| */ | ||
|
|
||
| import { click, settled, visit, fillIn, currentURL, waitFor, findAll } from '@ember/test-helpers'; | ||
| import { | ||
| click, | ||
| settled, | ||
| visit, | ||
| fillIn, | ||
| currentURL, | ||
| waitFor, | ||
| findAll, | ||
| triggerKeyEvent, | ||
| find, | ||
| } from '@ember/test-helpers'; | ||
| import { module, test } from 'qunit'; | ||
| import { setupApplicationTest } from 'ember-qunit'; | ||
| import { runCmd, createNS } from 'vault/tests/helpers/commands'; | ||
|
|
@@ -28,51 +38,83 @@ module('Acceptance | Enterprise | namespaces', function (hooks) { | |
| fetchSpy.restore(); | ||
| }); | ||
|
|
||
| test('it focuses the search input field when the component is loaded', async function (assert) { | ||
| assert.expect(1); | ||
|
||
|
|
||
| await click(NAMESPACE_PICKER_SELECTORS.toggle); | ||
|
|
||
| // Verify that the search input field is focused | ||
| const searchInput = find(NAMESPACE_PICKER_SELECTORS.searchInput); | ||
| assert.strictEqual( | ||
| document.activeElement, | ||
| searchInput, | ||
| 'The search input field is focused on component load' | ||
| ); | ||
| }); | ||
|
|
||
| test('it navigates to the matching namespace when Enter is pressed', async function (assert) { | ||
| assert.expect(2); | ||
|
|
||
| await click(NAMESPACE_PICKER_SELECTORS.toggle); | ||
|
|
||
| // Simulate typing into the search input | ||
| await fillIn(NAMESPACE_PICKER_SELECTORS.searchInput, 'beep/boop'); | ||
| assert | ||
| .dom(NAMESPACE_PICKER_SELECTORS.searchInput) | ||
| .hasValue('beep/boop', 'The search input field has the correct value'); | ||
|
|
||
| // Simulate pressing Enter | ||
| await triggerKeyEvent(NAMESPACE_PICKER_SELECTORS.searchInput, 'keydown', 'Enter'); | ||
|
|
||
| // Verify navigation to the matching namespace | ||
| assert.strictEqual( | ||
| this.owner.lookup('service:router').currentURL, | ||
| '/vault/dashboard?namespace=beep%2Fboop', | ||
| 'Navigates to the correct namespace when Enter is pressed' | ||
| ); | ||
| }); | ||
|
|
||
| test('it filters namespaces based on search input', async function (assert) { | ||
| assert.expect(7); | ||
| assert.expect(6); | ||
|
|
||
| await click(NAMESPACE_PICKER_SELECTORS.toggle); | ||
|
|
||
| // Verify all namespaces are displayed initially | ||
| assert.dom(NAMESPACE_PICKER_SELECTORS.link()).exists('Namespace link(s) exist'); | ||
| assert.strictEqual( | ||
| findAll(NAMESPACE_PICKER_SELECTORS.link()).length, | ||
| 5, | ||
| 'All namespaces are displayed initially' | ||
| ); | ||
| const allNamespaces = findAll(NAMESPACE_PICKER_SELECTORS.link()); | ||
|
|
||
| // Verify the search input field exists | ||
| assert.dom('[type="search"]').exists('The namespace search field exists'); | ||
| assert.dom(NAMESPACE_PICKER_SELECTORS.searchInput).exists('The namespace search field exists'); | ||
|
|
||
| // Verify 3 namespaces are displayed after searching for "beep" | ||
| await fillIn('[type="search"]', 'beep'); | ||
| await fillIn(NAMESPACE_PICKER_SELECTORS.searchInput, 'beep'); | ||
| assert.strictEqual( | ||
| findAll(NAMESPACE_PICKER_SELECTORS.link()).length, | ||
| 3, | ||
| 'Display 3 namespaces matching "beep" after searching' | ||
| ); | ||
|
|
||
| // Verify 1 namespace is displayed after searching for "bop" | ||
| await fillIn('[type="search"]', 'bop'); | ||
| await fillIn(NAMESPACE_PICKER_SELECTORS.searchInput, 'bop'); | ||
| assert.strictEqual( | ||
| findAll(NAMESPACE_PICKER_SELECTORS.link()).length, | ||
| 1, | ||
| 'Display 1 namespace matching "bop" after searching' | ||
| ); | ||
|
|
||
| // Verify no namespaces are displayed after searching for "other" | ||
| await fillIn('[type="search"]', 'other'); | ||
| await fillIn(NAMESPACE_PICKER_SELECTORS.searchInput, 'other'); | ||
| assert.strictEqual( | ||
| findAll(NAMESPACE_PICKER_SELECTORS.link()).length, | ||
| 0, | ||
| 'No namespaces are displayed after searching for "other"' | ||
| ); | ||
|
|
||
| // Clear the search input & verify all namespaces are displayed again | ||
| await fillIn('[type="search"]', ''); | ||
| await fillIn(NAMESPACE_PICKER_SELECTORS.searchInput, ''); | ||
| assert.strictEqual( | ||
| findAll(NAMESPACE_PICKER_SELECTORS.link()).length, | ||
| 5, | ||
| allNamespaces.length, | ||
| 'All namespaces are displayed after clearing search input' | ||
| ); | ||
| }); | ||
|
|
@@ -154,6 +196,7 @@ module('Acceptance | Enterprise | namespaces', function (hooks) { | |
|
|
||
| // check that the full namespace path, like "beep/boop", shows in the toggle display | ||
| await waitFor(NAMESPACE_PICKER_SELECTORS.link(targetNamespace)); | ||
|
|
||
| assert | ||
| .dom(NAMESPACE_PICKER_SELECTORS.link(targetNamespace)) | ||
| .hasText(targetNamespace, `shows the namespace ${targetNamespace} in the toggle component`); | ||
|
|
@@ -170,14 +213,12 @@ module('Acceptance | Enterprise | namespaces', function (hooks) { | |
| await waitFor(`svg${GENERAL.icon('check')}`); | ||
|
|
||
| // Find the selected element with the check icon & ensure it exists | ||
| const checkIcon = document.querySelector( | ||
| `${NAMESPACE_PICKER_SELECTORS.link()} svg${GENERAL.icon('check')}` | ||
| ); | ||
| assert.ok(checkIcon, 'A selected namespace link with the check icon exists'); | ||
| const checkIcon = find(`${NAMESPACE_PICKER_SELECTORS.link()} ${GENERAL.icon('check')}`); | ||
| assert.dom(checkIcon).exists('A selected namespace link with the check icon exists'); | ||
|
|
||
| // Get the selected namespace with the data-test-namespace-link attribute & ensure it exists | ||
| const selectedNamespace = checkIcon.closest(NAMESPACE_PICKER_SELECTORS.link()); | ||
| assert.ok(selectedNamespace, 'The selected namespace link exists'); | ||
| const selectedNamespace = checkIcon?.closest(NAMESPACE_PICKER_SELECTORS.link()); | ||
| assert.dom(selectedNamespace).exists('The selected namespace link exists'); | ||
|
|
||
| // Verify that the selected namespace has the correct data-test-namespace-link attribute and path value | ||
| assert.strictEqual( | ||
|
|
||
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 guessing that the focus on opening this was previous behavior? If so, I can't say we always did focus things according to best a11y practices. Looking at the hds pattens on focus, it might be good to review that we can indeed follow them.
If this was old behavior, maybe we should revisit this with Design's input (not blocking here, but come back with a ticket after discussion)? What are your thoughts?
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 linking this! previously there was no focus behavior set because there was no search input field in the namespace picker. but it feels natural that, when the user opens the namespace picker, focus should default to the search input. it potentially reduces clicks, allowing the user to immediately type in the search or tab to the namespaces in the list. I don't see anything re: this specific use case outlined in the a11y: focus management section, but it also doesn't seem to contradict the patterns outlined.