Refs #30784 - MemoryAllocation use react helper #8413

ezr-ondrej · 2021-03-26T00:26:01Z

In 8a9922f we've removed byte_size_f last usage.
This is moving the byte_size_f to render the React component, to
seamlessly integrate into the form without breaking any plugins that
were possibly still using it.

Adds posibility for InputFactory components to set validation states.

theforeman-bot · 2021-03-26T00:26:17Z

Issues: #30784

ezr-ondrej · 2021-03-26T00:38:17Z

webpack/assets/javascripts/compute_resource/ovirt.js

@@ -127,6 +103,16 @@ export function instanceTypeSelected(item) {
  }
 }

+function setMemoryInputProps(props) {


Simplifies all the usages.

ezr-ondrej · 2021-03-26T00:39:11Z

webpack/assets/javascripts/react_app/components/MemoryAllocationInput/MemoryAllocationInput.js

-      </Row>
-    </Grid>
+    <>
+      <RCInputNumber


Uses the external component directly, as the FormInput component takes care of the labels and other stuff.

yifatmakias

@ezr-ondrej Thanks for the improvements :)
but I noticed a few problems:

the label is missing both in the ovirt and libvirt pages.
in the ovirt form whenever you choose a template or instance type it should change the value in the memory input accordingly. It is not working after these changes.
the compute_profile_js_test is failing.

Ron-Lavi · 2021-04-11T13:17:33Z

app/views/compute_resources_vms/form/ovirt/_base.html.erb

@@ -39,12 +39,7 @@ disable_mem = !new_vm || (new_vm && instance_type && instance_type.memory.presen

 <%= counter_f f, :sockets, :disabled => disable_sockets, :label => _('Sockets'), :label_size => 'col-md-2' %>
 <div id="memory-input">


Is the wrapper div still needed?

yes, we are using it to select it and change the props (value) from legacy JS

Yes, we need to select it from the jquery code, so there is unfortunatelly no other way then add the wrapper.

Ron-Lavi · 2021-04-11T15:37:11Z

app/helpers/form_helper.rb

-    options[:help_block] += f.hidden_field(attr, :class => "real-hidden-value", :id => nil)
-
-    text_f(f, attr, options)
+    react_form_input('memory', f, attr, options)


looks good to me, but maybe plugin owners that use it can help test if it also works for them?
@tristanrobert mind take a look for foreman_fog_proxmox, there are several usages there
@shiramax mind take a look for foreman_kubevirt?

If those are the only use cases, @ezr-ondrej how about using only react_form_input so we won't need to maintain another wrapper?

I think the wrapper is very simple and beneficial - now we are free to change the implementation and the plugins are not required to do any changes in their code, if we would decided we want to implement this component in angular in the future, we can do so.

Ron-Lavi · 2021-04-11T15:40:46Z

webpack/assets/javascripts/compute_resource/ovirt.js

-            defaultValue: result.memory / megabyte,
-          })
-        );
+        setMemoryInputProps({ value: result.memory / megabyte });


how about storing it as a helper at the top of the file, and using it also on line 81, e.g:
const setMemoryValue = () => setMemoryInputProps({ value: result.memory / megabyte });

Sure, why not :) I think more like:
const setMemoryValue = memoryBytes => setMemoryInputProps({ value: memoryBytes / megabyte });

I've changed the component so it accepts value in bytes, so we don't need the calculation, and thus the wrapper seemed not very useful.

ezr-ondrej · 2021-04-15T11:23:39Z

Thanks @yifatmakias for testing, I belive I've fixed all the issues, could you retest?
@LaViro I've answered your comments, could you take another look? :)

Ron-Lavi

Thanks @ezr-ondrej ! code wise looks good to me 🚀
didn't test yet as I have some env issues, though I count on @yifatmakias with that :)

yifatmakias · 2021-04-18T09:49:22Z

@ezr-ondrej I tested it, works for me. Thanks

MariaAga

One small request before merging :)

...sets/javascripts/react_app/components/MemoryAllocationInput/MemoryAllocationInput.stories.js

In 8a9922f we've removed `byte_size_f` last usage. This is moving the byte_size_f to render the React component, to seamlessly integrate into the form without breaking any plugins that were possibly still using it. Adds posibility for InputFactory components to set validation states.

MariaAga

Thanks Ondrej!

tristanrobert · 2021-10-26T15:34:12Z

byte_size_f in fieldset_tag is no more enabled in form: field is ignored in form and controller. foreman_fog_proxmox plugin views using it are broken.

ezr-ondrej · 2021-10-26T20:30:45Z

byte_size_f in fieldset_tag is no more enabled in form: field is ignored in form and controller. foreman_fog_proxmox plugin views using it are broken.

Could you share more details, I believe it is working in the foreman usecases and those didn't change in view nor in the controller, so there have to be something special on the foreman_fog_proxmox usecase, would you happen to know what that is?

tristanrobert · 2021-10-27T10:09:46Z

For example, this view: https://github.com/theforeman/foreman_fog_proxmox/blob/91ca57f45944b5ce503c4276a7859374b41d671f/app/views/compute_resources_vms/form/proxmox/server/_volume_hard_disk.html.erb#L31
is called by HostsController#interfaces. But the form host parameters in the controller do not include host[compute_attributes][volumes_attributes][0][size], other fields are included except this one because it is a byte_size_f nested in a fieldset_tag. If it is moved outside the fieldset it works and it is in the parameters form.

tristanrobert · 2021-10-27T10:11:31Z

So I think I need to replace my fieldsets with div, but it is a huge change (js, views, etc). I use fieldsets to hide or disable many grouped fields in forms.

ezr-ondrej · 2021-11-04T10:26:15Z

So I think I need to replace my fieldsets with div, but it is a huge change (js, views, etc). I use fieldsets to hide or disable many grouped fields in forms.

I've tried to nest some byte_size_f under fieldset_tag, but it still worked. It's little bit weird, can't that be something about the array? or the double nesting?

tristanrobert · 2021-11-16T15:09:31Z

Which array? You are right it is about the nested fieldset.

theforeman-bot added Needs testing Not yet reviewed UI labels Mar 26, 2021

ezr-ondrej force-pushed the improve_memory_allocation branch from 589e45d to 61916d0 Compare March 26, 2021 00:36

ezr-ondrej commented Mar 26, 2021

View reviewed changes

ezr-ondrej force-pushed the improve_memory_allocation branch from 61916d0 to 44a38e6 Compare March 26, 2021 11:36

yifatmakias suggested changes Apr 5, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Apr 5, 2021

ezr-ondrej assigned MariaAga Apr 7, 2021

Ron-Lavi reviewed Apr 11, 2021

View reviewed changes

ezr-ondrej force-pushed the improve_memory_allocation branch from 44a38e6 to d007376 Compare April 15, 2021 10:53

theforeman-bot added Needs re-review and removed Waiting on contributor labels Apr 15, 2021

ezr-ondrej force-pushed the improve_memory_allocation branch from d007376 to 43f3ee3 Compare April 15, 2021 11:13

Ron-Lavi previously approved these changes Apr 18, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 18, 2021

yifatmakias mentioned this pull request Apr 18, 2021

Fixes #32272 - Deprecate jquery.ui.custom_spinners #8436

Merged

MariaAga requested changes Apr 19, 2021

View reviewed changes

...sets/javascripts/react_app/components/MemoryAllocationInput/MemoryAllocationInput.stories.js Outdated Show resolved Hide resolved

theforeman-bot added the Waiting on contributor label Apr 19, 2021

ezr-ondrej dismissed Ron-Lavi’s stale review via dd3eb10 April 19, 2021 07:51

ezr-ondrej force-pushed the improve_memory_allocation branch from 43f3ee3 to dd3eb10 Compare April 19, 2021 07:51

theforeman-bot added Needs re-review and removed Waiting on contributor labels Apr 19, 2021

MariaAga approved these changes Apr 19, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 19, 2021

MariaAga merged commit b264a1e into theforeman:develop Apr 19, 2021

ezr-ondrej deleted the improve_memory_allocation branch April 20, 2021 07:17

tristanrobert mentioned this pull request Oct 26, 2021

Create host fails on 2.5.2 theforeman/foreman_fog_proxmox#207

Closed

jacac mentioned this pull request Nov 17, 2022

Is this project still maintained? theforeman/foreman_fog_proxmox#227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #30784 - MemoryAllocation use react helper #8413

Refs #30784 - MemoryAllocation use react helper #8413

ezr-ondrej commented Mar 26, 2021

theforeman-bot commented Mar 26, 2021

ezr-ondrej Mar 26, 2021

ezr-ondrej Mar 26, 2021

yifatmakias left a comment

Ron-Lavi Apr 11, 2021

ezr-ondrej Apr 12, 2021

ezr-ondrej Apr 15, 2021

Ron-Lavi Apr 11, 2021

ezr-ondrej Apr 15, 2021 •

edited

Ron-Lavi Apr 11, 2021

ezr-ondrej Apr 15, 2021

ezr-ondrej Apr 15, 2021 •

edited

Ron-Lavi Apr 18, 2021

ezr-ondrej commented Apr 15, 2021

Ron-Lavi left a comment

yifatmakias commented Apr 18, 2021

MariaAga left a comment

MariaAga left a comment

tristanrobert commented Oct 26, 2021

ezr-ondrej commented Oct 26, 2021

tristanrobert commented Oct 27, 2021 •

edited

tristanrobert commented Oct 27, 2021 •

edited

ezr-ondrej commented Nov 4, 2021

tristanrobert commented Nov 16, 2021 •

edited

		@@ -39,12 +39,7 @@ disable_mem = !new_vm \|\| (new_vm && instance_type && instance_type.memory.presen

		<%= counter_f f, :sockets, :disabled => disable_sockets, :label => _('Sockets'), :label_size => 'col-md-2' %>
		<div id="memory-input">

Refs #30784 - MemoryAllocation use react helper #8413

Refs #30784 - MemoryAllocation use react helper #8413

Conversation

ezr-ondrej commented Mar 26, 2021

theforeman-bot commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifatmakias left a 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

ezr-ondrej Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej commented Apr 15, 2021

Ron-Lavi left a comment

Choose a reason for hiding this comment

yifatmakias commented Apr 18, 2021

MariaAga left a comment

Choose a reason for hiding this comment

MariaAga left a comment

Choose a reason for hiding this comment

tristanrobert commented Oct 26, 2021

ezr-ondrej commented Oct 26, 2021

tristanrobert commented Oct 27, 2021 • edited

tristanrobert commented Oct 27, 2021 • edited

ezr-ondrej commented Nov 4, 2021

tristanrobert commented Nov 16, 2021 • edited

ezr-ondrej Apr 15, 2021 •

edited

ezr-ondrej Apr 15, 2021 •

edited

tristanrobert commented Oct 27, 2021 •

edited

tristanrobert commented Oct 27, 2021 •

edited

tristanrobert commented Nov 16, 2021 •

edited