-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI bug fix: Kubernetes Role filter replace with explicit input filter #27178
Changes from 8 commits
b8c63c0
c0a9b80
d32e5b4
0858afc
75f05ae
6a17f8c
f373604
8356a81
128de02
47ded67
fb8c86a
24f3420
1823fc9
48477e5
dd63dfd
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 |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:change | ||
ui/kubernetes: Update the roles filter-input to use explicit search. | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
{{! | ||
Copyright (c) HashiCorp, Inc. | ||
SPDX-License-Identifier: BUSL-1.1 | ||
~}} | ||
|
||
<div> | ||
<Hds::SegmentedGroup as |S|> | ||
<S.TextInput | ||
@value={{@query}} | ||
placeholder={{@placeholder}} | ||
aria-label="Search by path" | ||
size="32" | ||
{{on "input" @handleInput}} | ||
{{on "keydown" @handleKeyDown}} | ||
data-test-filter-input-explicit | ||
/> | ||
<S.Button | ||
@color="secondary" | ||
@text="Search" | ||
@icon="search" | ||
type="submit" | ||
{{on "click" @handleSearch}} | ||
data-test-filter-input-explicit-search | ||
/> | ||
</Hds::SegmentedGroup> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/** | ||
* Copyright (c) HashiCorp, Inc. | ||
* SPDX-License-Identifier: BUSL-1.1 | ||
*/ | ||
|
||
export { default } from 'core/components/filter-input-explicit'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,24 +9,64 @@ import { action } from '@ember/object'; | |
import { getOwner } from '@ember/application'; | ||
import errorMessage from 'vault/utils/error-message'; | ||
import { tracked } from '@glimmer/tracking'; | ||
import keys from 'core/utils/key-codes'; | ||
|
||
/** | ||
* @module Roles | ||
* RolesPage component is a child component to show list of roles | ||
* RolesPage component is a child component to show list of roles. | ||
* It also handles the filtering actions of roles. | ||
* | ||
* @param {array} roles - array of roles | ||
* @param {boolean} promptConfig - whether or not to display config cta | ||
* @param {array} pageFilter - array of filtered roles | ||
* @param {string} filterValue - value of queryParam pageFilter | ||
* @param {array} breadcrumbs - breadcrumbs as an array of objects that contain label and route | ||
*/ | ||
export default class RolesPageComponent extends Component { | ||
@service flashMessages; | ||
@service router; | ||
@tracked query; | ||
@tracked roleToDelete = null; | ||
|
||
constructor() { | ||
super(...arguments); | ||
this.query = this.args.filterValue; | ||
hashishaw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
get mountPoint() { | ||
return getOwner(this).mountPoint; | ||
} | ||
|
||
navigate(pageFilter) { | ||
const route = `${this.mountPoint}.roles.index`; | ||
const args = [route]; | ||
|
||
args.push({ | ||
queryParams: { | ||
pageFilter: pageFilter ? pageFilter : null, | ||
}, | ||
}); | ||
this.router.transitionTo(...args); | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@action | ||
handleKeyDown(event) { | ||
if (event.keyCode === keys.ESC) { | ||
// On escape, transition to roles index route. | ||
this.navigate(); | ||
} | ||
// ignore all other key events | ||
Comment on lines
+48
to
+51
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 slick! nice job including keyboard navigation here. 😀 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 originally comes from Chelsea's refactor of an old filter input for Kv (see |
||
} | ||
|
||
@action handleInput(evt) { | ||
this.query = evt.target.value; | ||
} | ||
|
||
@action | ||
handleSearch(evt) { | ||
evt.preventDefault(); | ||
this.navigate(this.query); | ||
} | ||
|
||
@action | ||
async onDelete(model) { | ||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ export const GENERAL = { | |
|
||
filter: (name: string) => `[data-test-filter="${name}"]`, | ||
filterInput: '[data-test-filter-input]', | ||
filterInputExplicit: '[data-test-filter-input-explicit]', | ||
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. 👏 |
||
filterInputExplicitSearch: '[data-test-filter-input-explicit-search]', | ||
confirmModalInput: '[data-test-confirmation-modal-input]', | ||
confirmButton: '[data-test-confirm-button]', | ||
confirmTrigger: '[data-test-confirm-action-trigger]', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
/** | ||
* Copyright (c) HashiCorp, Inc. | ||
* SPDX-License-Identifier: BUSL-1.1 | ||
*/ | ||
|
||
import { module, test } from 'qunit'; | ||
import { setupRenderingTest } from 'ember-qunit'; | ||
import { render, typeIn, click } from '@ember/test-helpers'; | ||
import hbs from 'htmlbars-inline-precompile'; | ||
import { GENERAL } from 'vault/tests/helpers/general-selectors'; | ||
import sinon from 'sinon'; | ||
|
||
module('Integration | Component | filter-input-explicit', function (hooks) { | ||
setupRenderingTest(hooks); | ||
|
||
hooks.beforeEach(function () { | ||
this.handleSearch = sinon.spy(); | ||
this.handleInput = sinon.spy(); | ||
this.handleKeyDown = sinon.spy(); | ||
this.query = ''; | ||
this.placeholder = 'Filter roles'; | ||
|
||
this.renderComponent = () => { | ||
return render( | ||
hbs`<FilterInputExplicit aria-label="test-component" @placeholder={{this.placeholder}} @query={{this.query}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}} />` | ||
); | ||
}; | ||
}); | ||
|
||
test('it renders', async function (assert) { | ||
assert.expect(2); | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.query = 'foo'; | ||
await this.renderComponent(); | ||
|
||
assert | ||
.dom(GENERAL.filterInputExplicit) | ||
.hasAttribute('placeholder', 'Filter roles', 'Placeholder passed to input element'); | ||
assert.dom(GENERAL.filterInputExplicit).hasValue('foo', 'Value passed to input element'); | ||
}); | ||
|
||
test('it should call handleSearch on submit', async function (assert) { | ||
assert.expect(1); | ||
|
||
this.handleSearch = () => { | ||
assert.ok(true, 'handleSearch was called'); | ||
}; | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
await this.renderComponent(); | ||
await typeIn(GENERAL.filterInputExplicit, 'bar'); | ||
await click(GENERAL.filterInputExplicitSearch); | ||
}); | ||
|
||
test('it should send keydown event on keydown', async function (assert) { | ||
assert.expect(3); // handle key down called twice | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
this.handleKeyDown = () => { | ||
assert.ok(true, 'handleKeydown was called'); | ||
}; | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
await this.renderComponent(); | ||
await typeIn(GENERAL.filterInputExplicit, 'a'); | ||
await typeIn(GENERAL.filterInputExplicit, 'b'); | ||
|
||
assert.ok(this.handleSearch.notCalled); | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
}); |
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.
Is there a reason we aren't wrapping these in a
<form>
element so that clicking this button submits (and then we also get a free submit when the user presses the "Enter" button)? I think that would be a nice ergonomic update to this.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.
The tests 😵💫 I originally had it wrapped as a form, but couldn't call the action correctly from the tests. There was an error about it being in a form element. I wish I would have saved that specific error. Let me see if I can resurface it. The enter functionality would be very nice!
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.
@hashishaw This was the error, any thoughts? Only on the test, not on the UI.

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.
oh, we just need to do
evt.preventDefault
in the triggered action so that it doesn't "send" the form data to the URLThere 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.
Got it! Found an example in how we do it for sinon spies in toggle-test (thank you—you did that block of code).