-
Notifications
You must be signed in to change notification settings - Fork 2k
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] Multi-line variable values and helios upgrades generally #19544
Conversation
Ember Test Audit comparison
|
9f521bc
to
554ae46
Compare
4feea33
to
ec15ec7
Compare
ui/app/components/variable-form.hbs
Outdated
> | ||
Add More | ||
</button> | ||
<Hds::Button @text="Add More" @color="secondary" @size="medium" @icon="plus" class="add-more" type="button" disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} {{on "click" this.appendRow}} data-test-add-kv /> |
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.
This long disabled
attr says "Don't let the user add more fields if their last one is currently empty"
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.
This translation helps, thanks!
little style nit: sometimes I like (some of) the attributes being placed on separate lines, for big mouths-full like this. I still might not know how to interpret the semantics of the frontent magic, but some whitespace can help me follow the syntax, e.g.
<Hds::Button @text="Add More" @color="secondary" @size="medium" @icon="plus" class="add-more" type="button" disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} {{on "click" this.appendRow}} data-test-add-kv /> | |
<Hds::Button | |
@text="Add More" @color="secondary" @size="medium" @icon="plus" | |
class="add-more" type="button" | |
{{! disabled if the bottom variable's key or value boxes are empty }} | |
disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} | |
{{on "click" this.appendRow}} | |
data-test-add-kv | |
/> |
and makes room to include that interesting {{! comment }}
syntax to leave notes in the code.
Is there a reason not to newline-separate the bits like this?
Also, now arranged like that, I wonder: is class="add-more" type="button"
needed at all? Isn't it being supplanted by <Hds::Button ... @icon="plus"
?
@@ -207,6 +208,7 @@ export default class VariableFormComponent extends Component { | |||
* @param {KeyboardEvent} e | |||
*/ | |||
@action setModelPath(e) { | |||
set(this, 'path', e.target.value); |
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.
This, and the entry
set above it, are side-effects of the Helios input component not doing two-way data binding. For some additional context, two-way data binding is kind of a bygone convention in most JS frameworks at this point so it's not that weird, but we were using that bygone convention for quite awhile. Now, we are explicit about "When you do something that indicates needing to save something, save it".
In this case, that happens in the template on keystroke ({{on "input" (action this.setModelPath)}}
), but it's also common to do this on element blur, on form submit, etc. We're just eager beavers here.
@columns={{array | ||
(hash | ||
key="key" | ||
label="Key" | ||
isSortable=true | ||
width="200px" | ||
) | ||
(hash | ||
key="value" | ||
label="Value" | ||
isSortable=true | ||
) | ||
}} |
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.
An astute observe may note that some of the <Hds::Table>
s here have a @columns
property, where others do the <:head as |H|>
and H.Tr
thing. Helios tables allow us to do either, and the best argument for doing the former has to do with sorting being baked in.
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.
lgtm
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 found some nits to pick, but nothing really blocky, so overall LGTM!
ui/app/components/variable-form.hbs
Outdated
> | ||
Add More | ||
</button> | ||
<Hds::Button @text="Add More" @color="secondary" @size="medium" @icon="plus" class="add-more" type="button" disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} {{on "click" this.appendRow}} data-test-add-kv /> |
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.
This translation helps, thanks!
little style nit: sometimes I like (some of) the attributes being placed on separate lines, for big mouths-full like this. I still might not know how to interpret the semantics of the frontent magic, but some whitespace can help me follow the syntax, e.g.
<Hds::Button @text="Add More" @color="secondary" @size="medium" @icon="plus" class="add-more" type="button" disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} {{on "click" this.appendRow}} data-test-add-kv /> | |
<Hds::Button | |
@text="Add More" @color="secondary" @size="medium" @icon="plus" | |
class="add-more" type="button" | |
{{! disabled if the bottom variable's key or value boxes are empty }} | |
disabled={{not (and this.keyValues.lastObject.key this.keyValues.lastObject.value)}} | |
{{on "click" this.appendRow}} | |
data-test-add-kv | |
/> |
and makes room to include that interesting {{! comment }}
syntax to leave notes in the code.
Is there a reason not to newline-separate the bits like this?
Also, now arranged like that, I wonder: is class="add-more" type="button"
needed at all? Isn't it being supplanted by <Hds::Button ... @icon="plus"
?
@gulducat growing textarea on input is a good idea here. Let me see if the Helios masked input can support Update: made an issue for it |
f05fef7
to
a49391b
Compare
This started as an attempt to allow multi-line variable editing in the UI (#19543) and turned into a larger effort to replace the Variables UI's use of common components with Helios components
Resolves #19543