Masked input #4759
Masked input #4759
Conversation
There was a problem hiding this comment.
This is nice! 👍 I left some minor code style notes inline, nothing important.
One other thing: it looks like the integration test doesn't cover the displayOnly configuration of the component, which is how it seems to be used in the app. Would it be reasonable to add tests for that, too?
ui/app/components/masked-input.js
Outdated
| didRender(){ | ||
| this._super(...arguments); | ||
| autosize.update(this.element.querySelector('textarea')); | ||
| }, |
There was a problem hiding this comment.
The didRender hook is also fired on initial render. Maybe you want didUpdate here? Component lifecycle hooks docs here
ui/app/components/masked-input.js
Outdated
| onChange(){}, | ||
| updateValue(e){ | ||
| this.set('value', e.target.value); | ||
| this.get('onChange')(); |
There was a problem hiding this comment.
This could be written as this.onChange(), which might be a little easier to read.
| wrap="off" | ||
| placeholder="value" | ||
| onfocus={{action "setFocus" true}} | ||
| onblur={{action "setFocus" false}} |
There was a problem hiding this comment.
I think these could be {{action (mut setFocus) true}} and …false}}, removing the need for the action.
| onfocus={{action "setFocus" true}} | ||
| onblur={{action "setFocus" false}} | ||
| onkeydown={{action onKeyDown}} | ||
| onchange={{action updateValue}} |
There was a problem hiding this comment.
I think the action helpers here are unneeded, as these attributes are already bound closure actions.
| enterText: fillable('[data-test-textarea]'), | ||
| toggleMasked: clickable('[data-test-button]'), | ||
| focus: focusable('[data-test-textarea]'), | ||
| blur: blurrable('[data-test-textarea]') |
There was a problem hiding this comment.
I did not know that focusable and blurrable were ecpo functions! Super useful.
ui/tests/acceptance/console-test.js
Outdated
| andThen(() => { | ||
| assert.equal(enginesPage.rows().count, numEngines + 3, 'new engines were added to the page'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test seems unrelated to the rest of the changes. Am I missing something?
There was a problem hiding this comment.
🤔 something odd going on - this is already in master https://github.com/hashicorp/vault/commits/master/ui/tests/acceptance/console-test.js
Maybe just needs a rebase? Or was rebased oddly? Uncertain at this moment.
|
Can we make the toggle icon flush with the left column header? In secrets it's just the column header that is flush, but in other places the non-secret text might look odd with it indented
|
| .masked-input.masked .masked-value, | ||
| .masked-input.masked .masked-input-toggle { | ||
| color: $grey-light; | ||
| } |
There was a problem hiding this comment.
Some strange indenting going on in this file
There was a problem hiding this comment.
Default editor settings! Sorryyyyyy fix'd
| position: absolute; | ||
| height: auto; | ||
| top: $size-6/4; | ||
| bottom: $size-6/4; |
There was a problem hiding this comment.
Looking forward to the new spacing variables :\
madalynrose
force-pushed
the
masked-input
branch
from
e9d1bae to
20e6732
Compare
Fixes #4422
