Fixes #24599 - Not custom instance type should disable params #5945

shiramax · 2018-08-14T09:13:21Z

No description provided.

theforeman-bot · 2018-08-14T09:13:35Z

houndci-bot · 2018-08-14T09:15:13Z

webpack/assets/javascripts/compute_resource/ovirt.js

+          $('[id$=_cores]').attr('disabled', 'disabled');
+          $('[id$=_sockets]').attr('disabled', 'disabled');
+          $('[id$=_ha]').attr('disabled', 'disabled');
+        }


Closing curly brace does not appear on the same line as the subsequent block brace-style

lizagilman · 2018-08-14T15:41:05Z

tnx @shiramax ,
could you provide a screenshot of before & after?

shiramax · 2018-08-16T07:43:33Z

Thanks @lizagilman
Before :

After:

shiramax · 2018-08-16T07:46:34Z

also the instance type text was changed:
before:

after:

lizagilman · 2018-08-16T08:51:38Z

I find the current default values for Template and Instance type fields a bit confusing.
Currently the options are:

For Template field -

"Select template" (current default)
Blank
and a list of available templates

For Instance Type field-
"Select instance type" (current default)
and a list of available instances

Basically "Select Template" and "Blank" has the same behavior - no auto updating of any other fields.
So if a user proceeds on with filling out the form, why are they told to "Select Template" ?

I can think of three other options for default values.

For Template -

"No template selected"
"Blank"
just display an empty select field

For Instance type -

"Costume instance type" (as suggested in this pr)
"No instance type selected"
just display an empty select field

also should the field names and the option names, or some of them, be titlized?

@Rohoover / @terezanovotna could you advise here?

terezanovotna · 2018-08-16T09:14:01Z

Hi @lizagilman,

I am looking on the PF Multiple Select component, how about using "Nothing Selected" for both template and instance type?

lizagilman · 2018-08-16T09:23:11Z

Hi @terezanovotna tnx for the reply

Sounds good for the Template field
Not sure about the Instance Type field, because when nothing is selected, then it's a costume instance.

shiramax · 2018-08-16T11:28:42Z

@lizagilman, @terezanovotna
The template and instance type should reflect the template and instance type in ovirt.
"Nothing selected" is not the correct behavior in Ovirt for the template and for instance type.
If nothing is selected by the user, the default template selected is "blank" template
the same for instance type, if the user doesn't select an instance type, then the default one is "custom":

terezanovotna · 2018-08-16T13:29:40Z

Thanks for the explanation @shiramax that is helpful!

If it is default and already has a name, could we name it:

Default - Blank
Default - Custom

Would that make sense? The user knows it is default and if he/she wants select something different, he/she can.

shiramax · 2018-08-16T13:55:45Z

@lizagilman @terezanovotna , I'm not loving the prefix "Default-"
Why shouldn't we write it the same it's in ovirt?

terezanovotna · 2018-08-16T14:46:10Z

@shiramax Do you think it is clear for the user that Blank and Custom are default? If so, it's fine to use it. I want to avoid the situation when the user starts wondering what Blank and Custom actually mean

Rohoover · 2018-08-16T17:36:30Z

I agree with @shiramax

We should mimic ovirt. Are these optional fields or required?

Another option would be to make the fields required, keep "Select XX" but force a selection. "Select XX" should not be a clickable option.

lizagilman · 2018-08-20T07:05:22Z

webpack/assets/javascripts/compute_resource/ovirt.js

+          $('[id$=_memory]').attr('disabled', 'disabled');
+          $('[id$=_cores]').attr('disabled', 'disabled');
+          $('[id$=_sockets]').attr('disabled', 'disabled');
+          $('[id$=_ha]').attr('disabled', 'disabled');


If you would like, you can do something like [ $('[id$=_memory]'), $('[id$=_cores]'), $('[id$=_sockets]'), $('[id$=_ha]') ].forEach( ()=> $(this).attr('disabled', 'disabled') })

lizagilman · 2018-08-21T08:26:49Z

[test foreman]

tbrisker · 2018-08-22T09:06:49Z

webpack/assets/javascripts/compute_resource/ovirt.js

+          $('[id$=_cores]').removeAttr('disabled');
+          $('[id$=_sockets]').removeAttr('disabled');
+          $('[id$=_ha]').removeAttr('disabled');
+        }


Please use .prop('disabled', <true or false>) instead of attr and removeAttr. Building on @lizagilman's suggestion, you can do it all in one:

[ _'memory', '_cores', '_sockets', '_ha'].forEach((name)=> $(`[id$=${name}]`).prop('disabled', (result.name != null)))

houndci-bot · 2018-08-27T13:52:57Z

webpack/assets/javascripts/compute_resource/ovirt.js

+            $('[id$=_sockets]').val(result.sockets);
+            $('[id$=_ha]').prop('checked', result.ha);
+        }
+        [ '_memory', '_cores', '_sockets', '_ha'].forEach((name)=> $(`[id$=${name}]`).prop('readOnly', (result.name != null)))


There should be no space after '[' array-bracket-spacing
Unexpected parentheses around single function argument having a body with no curly braces arrow-parens
Missing space before => arrow-spacing
Missing semicolon semi

houndci-bot · 2018-08-27T13:52:57Z

webpack/assets/javascripts/compute_resource/ovirt.js

+            $('[id$=_memory]').val(result.memory).trigger('change');
+            $('[id$=_cores]').val(result.cores);
+            $('[id$=_sockets]').val(result.sockets);
+            $('[id$=_ha]').prop('checked', result.ha);


Expected indentation of 10 spaces but found 12 indent

houndci-bot · 2018-08-27T13:52:57Z

webpack/assets/javascripts/compute_resource/ovirt.js

+        if (result.name != null) {
+            $('[id$=_memory]').val(result.memory).trigger('change');
+            $('[id$=_cores]').val(result.cores);
+            $('[id$=_sockets]').val(result.sockets);


Expected indentation of 10 spaces but found 12 indent

houndci-bot · 2018-08-27T13:52:57Z

webpack/assets/javascripts/compute_resource/ovirt.js

-        $('[id$=_ha]').prop('checked', result.ha);
+        if (result.name != null) {
+            $('[id$=_memory]').val(result.memory).trigger('change');
+            $('[id$=_cores]').val(result.cores);


Expected indentation of 10 spaces but found 12 indent

houndci-bot · 2018-08-27T13:52:58Z

webpack/assets/javascripts/compute_resource/ovirt.js

-        $('[id$=_sockets]').val(result.sockets);
-        $('[id$=_ha]').prop('checked', result.ha);
+        if (result.name != null) {
+            $('[id$=_memory]').val(result.memory).trigger('change');


Expected indentation of 10 spaces but found 12 indent

houndci-bot · 2018-08-27T14:05:40Z

webpack/assets/javascripts/compute_resource/ovirt.js

        const instanceTypeSelector = $('#host_compute_attributes_instance_type');
-
+          


Trailing spaces not allowed no-trailing-spaces

shiramax · 2018-09-03T10:58:02Z

@lizagilman, @tbrisker please review again :)

lizagilman · 2018-09-03T12:15:30Z

LGTM 👍

tbrisker

Thanks @shiramax and @lizagilman !

theforeman-bot added Needs testing Not yet reviewed labels Aug 14, 2018

houndci-bot reviewed Aug 14, 2018

View reviewed changes

shiramax force-pushed the 24599 branch from 47cae5e to 5815f83 Compare August 14, 2018 10:41

lizagilman reviewed Aug 20, 2018

View reviewed changes

tbrisker reviewed Aug 22, 2018

View reviewed changes

shiramax force-pushed the 24599 branch from 5815f83 to 122b772 Compare August 27, 2018 13:52

houndci-bot reviewed Aug 27, 2018

View reviewed changes

shiramax force-pushed the 24599 branch from 122b772 to eea8385 Compare August 27, 2018 14:05

houndci-bot reviewed Aug 27, 2018

View reviewed changes

shiramax force-pushed the 24599 branch 2 times, most recently from 8f3b324 to 449e2cd Compare August 28, 2018 12:15

Fixes #24599 - Not custom instance type should disable params

43f804f

shiramax force-pushed the 24599 branch from 449e2cd to 43f804f Compare August 29, 2018 07:02

tbrisker approved these changes Sep 3, 2018

View reviewed changes

theforeman-bot added UI and removed Not yet reviewed labels Sep 3, 2018

tbrisker merged commit 735c73a into theforeman:develop Sep 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #24599 - Not custom instance type should disable params #5945

Fixes #24599 - Not custom instance type should disable params #5945

shiramax commented Aug 14, 2018

theforeman-bot commented Aug 14, 2018

houndci-bot Aug 14, 2018

lizagilman commented Aug 14, 2018

shiramax commented Aug 16, 2018

shiramax commented Aug 16, 2018

lizagilman commented Aug 16, 2018 •

edited

terezanovotna commented Aug 16, 2018

lizagilman commented Aug 16, 2018

shiramax commented Aug 16, 2018

terezanovotna commented Aug 16, 2018

shiramax commented Aug 16, 2018

terezanovotna commented Aug 16, 2018

Rohoover commented Aug 16, 2018

lizagilman Aug 20, 2018

lizagilman commented Aug 21, 2018

tbrisker Aug 22, 2018

houndci-bot Aug 27, 2018

houndci-bot Aug 27, 2018

houndci-bot Aug 27, 2018

houndci-bot Aug 27, 2018

houndci-bot Aug 27, 2018

houndci-bot Aug 27, 2018

shiramax commented Sep 3, 2018

lizagilman commented Sep 3, 2018

tbrisker left a comment

		const instanceTypeSelector = $('#host_compute_attributes_instance_type');

Fixes #24599 - Not custom instance type should disable params #5945

Fixes #24599 - Not custom instance type should disable params #5945

Conversation

shiramax commented Aug 14, 2018

theforeman-bot commented Aug 14, 2018

Choose a reason for hiding this comment

lizagilman commented Aug 14, 2018

shiramax commented Aug 16, 2018

shiramax commented Aug 16, 2018

lizagilman commented Aug 16, 2018 • edited

terezanovotna commented Aug 16, 2018

lizagilman commented Aug 16, 2018

shiramax commented Aug 16, 2018

terezanovotna commented Aug 16, 2018

shiramax commented Aug 16, 2018

terezanovotna commented Aug 16, 2018

Rohoover commented Aug 16, 2018

Choose a reason for hiding this comment

lizagilman commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiramax commented Sep 3, 2018

lizagilman commented Sep 3, 2018

tbrisker left a comment

Choose a reason for hiding this comment

lizagilman commented Aug 16, 2018 •

edited