Replaced ror form with react form component #3856

Hyperkid123 · 2018-04-26T12:22:32Z

compute -> infrastructure -> virtual machines (or any other VM)
select virtual machine, click on snapshot row in table
click on add snapshot button in top toolbar

fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1527371
component is implemented in react-ui-components

There was no validation on client side, so the form had to be rewritten in JS.

added react-ui-components to dependencies

Before

After

When the i18n PR in react-ui-components is merged, will remove the strings from props.

Hyperkid123 · 2018-04-26T12:23:07Z

@miq-bot assign @martinpovolny
@miq-bot add_label wip

ohadlevy · 2018-04-26T12:33:52Z

app/views/vm_common/_snap.html.haml

+        if(values.snap_memory) {
+          values.snap_memory = 1;
+        }
+        miqAjaxButton('#{url_for_only_path(:action => "snap_vm", :id => @record.id, :button => @edit ? "create" : nil)}', values);


why split the logic? and have it inline? I would expect that if we move something to react, we don't use haml anymore?

ohadlevy · 2018-04-26T12:34:29Z

app/javascript/packs/application-common.js

@@ -16,6 +17,7 @@ import reactBlueprint from '../miq-component/react-blueprint';
 import * as helpers from '../miq-component/helpers';

 import { rxSubject, sendDataWithRx, listenToRx } from '../miq_observable';
+import { SnapshotForm } from '@manageiq/react-ui-components/dist/vm-snapshot-form';


imho better to split this logic to another file, we should try keeping application-common to a minimum / top level

Yep i agree this is in early phase i just needed to consult some things with @martinpovolny

ohadlevy · 2018-04-26T12:35:08Z

app/javascript/packs/application-common.js

@@ -31,3 +33,9 @@ ManageIQ.component = {
 ManageIQ.angular.rxSubject = rxSubject;
 window.sendDataWithRx = sendDataWithRx;
 window.listenToRx = listenToRx;
+
+$(() => {
+  newRegistry.define('SnapshotForm', reactBlueprint(props => (


why do you want to mount this on every page? imho the rails view should be trigging mounting the component instead?

Hyperkid123 · 2018-04-26T12:36:00Z

@ohadlevy yep i know i did this to mostly test the component. Refactoring is coming

Hyperkid123 · 2018-04-26T15:16:51Z

cc @karelhala

karelhala · 2018-04-27T11:34:12Z

@Hyperkid123 the problem you are facing is known issue and it's not trivial to be fixed. The problem is how loading of packs from ruby webpack works, he basically identifies if some packs should be loaded. Your pack was loaded in partial which is rendered on server and served as html partial containing only load par of such pack, however this pack is identified as not to load on such page.

When I went trough the code it looks like the best approach would be to change name of the pack to vm-snapshot-form-common.js so it' loaded on every page. Then have some class/id/attribute which you'd try to load from DOM if such thing is present run specific code for this form.

Hyperkid123 · 2018-04-30T06:38:36Z

@karelhala that sucks

ohadlevy · 2018-04-30T06:43:14Z

@Hyperkid123 I personally don't see a lot of value of creating many small packs, rather unify them and have only one server roundtrip (and also caching)?

ohadlevy · 2018-04-30T06:47:11Z

app/javascript/vm-snapshot-form/register.jsx

+  );
+};
+
+$(() => {


TBH I don't understand why we are solving the problem this way.

IMHO the right way to do this is:

expose the top level / high level react component so its available to the registry

use a rails view helper to mount the component on the dom as demonstrated at https://github.com/priley86/miq_v2v_ui_plugin/blob/master/app/views/migration/index.html.haml#L2
this will allow you to pass the right data in the place you would expect react to be rendering (Rather relaying on jquery functions).
You would be able to pass any ruby props at that stage as well.

@Hyperkid123 is not using ReactDOM.render directly, but trough component's API here so using mounter might not be that great. However we should use as less jquery as possible so perhaps rewriting $(() =>{}) into document.addEventListener('DOMContentLoaded', () => {}, false); might be better (or create helper function which does this thing, or even better use DocReady). And of course let's use document.querySelector[All] with getAttribute to access data attr.

Other way which might be better is to create new ruby helper function which does all these things combined.

Hyperkid123 · 2018-05-03T11:12:03Z

@karelhala @ohadlevy. This is what i have come up with.What do you think? @himdel advised, that i should create a smart wrapper around the component, that will pass manageiq specific data. I have also created a simple factory, that will build the component. That should remove the JQuery code. Rest of the props is passed from the element as data propeties. Name of these properties must match the component props.

ohadlevy · 2018-05-03T11:34:39Z

app/views/vm_common/_snap.html.haml

+           "data-name-required"  => !!@record.try(:snapshot_name_optional?)}
+
+:javascript
+  ManageIQ.component.componentFactory('VmSnapshotForm', 'snap-form');


why not simply pass json instead of an html? am i missing something or is this what https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/reactjs_helper.rb suppose to be doing.....?

We are using different React API #3633. It it slightly different than your mounter. Also why not JSON. Usign element.dataset will return JSON from data attributes, so its basically the same and that way we don't need additional ruby code.

I think its better to do it in ruby vs doing in html template (haml) if its going to be consumed as json at the end anyway, we go though an extra step?

Hyperkid123 · 2018-05-07T09:58:37Z

@martinpovolny can you check it out? From what i know, RHV VMs shoud have a bit different behavior than the rest. Also i have added a simple function that will initialize the component. It is very similar to the current angular way to initialize components.

ohadlevy · 2018-05-07T10:04:58Z

app/views/vm_common/_snap.html.haml

-            :title => t,
-            :type  => "submit")
+= render :partial => "layouts/flash_msg"
+#snap-form{"data-create-url" => url_for_only_path(:action => "snap_vm", :id => @record.id, :button => @edit ? "create" : nil),


I'll add it here again, I can't see value in using the dom as a place holder for data, we can avoid the extra dom lookup and putting data on it directly by simply passing json to the component factory.

was this done by design? /cc @himdel @vojtechszocs

Does it matter? I mean, you either put the data here, and deal with the data types, escapement with HAML and Ruby or you put the data in a hash, encode it in JSON and have it in the :javascript DOM node as code.

What's the benefit of one vs the other?

From the introspection perspective with this approach data stays data in the data-* attribute that is meant for data ;-) Is that wrong somehow?

@ohadlevy : I am trying to understand your concern so that we don't disregard your opinion. But I really don't see the problem. Neither does @himdel or @Hyperkid123.

From my perspective it would be best if the #snap-form... and the Manageiq.component.componentFactory.... statements where a single entity. I'd see value in that. But if these are to be 2 pieces then I don't see much difference if the data is in the former or the latter.

martinpovolny · 2018-05-10T07:37:23Z

app/views/vm_common/_snap.html.haml

-            :title => t,
-            :type  => "submit")
+= render :partial => "layouts/flash_msg"
+#snap-form{"data-create-url" => url_for_only_path(:action => "snap_vm", :id => @record.id, :button => @edit ? "create" : nil),


Does it matter? I mean, you either put the data here, and deal with the data types, escapement with HAML and Ruby or you put the data in a hash, encode it in JSON and have it in the :javascript DOM node as code.

What's the benefit of one vs the other?

From the introspection perspective with this approach data stays data in the data-* attribute that is meant for data ;-) Is that wrong somehow?

@ohadlevy : I am trying to understand your concern so that we don't disregard your opinion. But I really don't see the problem. Neither does @himdel or @Hyperkid123.

From my perspective it would be best if the #snap-form... and the Manageiq.component.componentFactory.... statements where a single entity. I'd see value in that. But if these are to be 2 pieces then I don't see much difference if the data is in the former or the latter.

miq-bot · 2018-05-14T11:32:35Z

This pull request is not mergeable. Please rebase and repush.

Hyperkid123 · 2018-06-07T07:53:41Z

@hstastna helped me to test this form in UI and it is working. I think it can be merged.

miq-bot · 2018-06-07T10:38:57Z

This pull request is not mergeable. Please rebase and repush.

Hyperkid123 · 2018-06-07T12:44:06Z

@miq-bot remove_label unmergeable

miq-bot · 2018-06-12T10:08:52Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2018-06-12T10:29:35Z

Checked commits Hyperkid123/manageiq-ui-classic@0766d37~...59627b7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

Hyperkid123 · 2018-06-12T10:48:57Z

@miq-bot remove_label unmergeable

miq-bot assigned martinpovolny Apr 26, 2018

miq-bot added the wip label Apr 26, 2018

ohadlevy reviewed Apr 26, 2018

View reviewed changes

ohadlevy reviewed Apr 30, 2018

View reviewed changes

Hyperkid123 force-pushed the snap-form branch from ba09138 to 2f2a227 Compare May 3, 2018 11:18

ohadlevy reviewed May 3, 2018

View reviewed changes

Hyperkid123 force-pushed the snap-form branch 2 times, most recently from f0c3060 to 5c14bdb Compare May 3, 2018 13:08

Hyperkid123 changed the title ~~[WIP] Replaced ror form with react form component~~ Replaced ror form with react form component May 7, 2018

Hyperkid123 changed the title ~~Replaced ror form with react form component~~ [WIP] Replaced ror form with react form component May 7, 2018

Hyperkid123 changed the title ~~[WIP] Replaced ror form with react form component~~ Replaced ror form with react form component May 7, 2018

miq-bot removed the wip label May 7, 2018

ohadlevy suggested changes May 7, 2018

View reviewed changes

martinpovolny approved these changes May 10, 2018

View reviewed changes

Hyperkid123 force-pushed the snap-form branch 2 times, most recently from 892e293 to 4b4b514 Compare May 14, 2018 11:20

miq-bot added the unmergeable label May 14, 2018

Hyperkid123 force-pushed the snap-form branch 2 times, most recently from 97221cf to fc6bc00 Compare May 14, 2018 11:46

miq-bot added the unmergeable label May 28, 2018

Hyperkid123 force-pushed the snap-form branch from 520e9e5 to 0af588b Compare May 31, 2018 09:23

miq-bot removed the unmergeable label May 31, 2018

miq-bot added the unmergeable label Jun 7, 2018

Hyperkid123 added 9 commits June 7, 2018 14:09

Replaced ror form with react form component

0766d37

Added helper for hiding specific partials.

b909459

Broken build for @karelhala

6e1c3b8

Added mounting process.

8534fbc

Small changes to fit the ui code.

6392813

Added react-ui-components to dependencies.

b907c5f

Switched to the new react helper

e127d5b

Using different notation for array in helper.

313774e

Cosmetic changes.

d7c3ed5

Hyperkid123 force-pushed the snap-form branch 2 times, most recently from 044663a to 0c2921d Compare June 7, 2018 12:42

miq-bot removed the unmergeable label Jun 7, 2018

Updated react-ui-components to latest verstion

7917666

Hyperkid123 force-pushed the snap-form branch from 0c2921d to 7917666 Compare June 7, 2018 13:01

miq-bot added the unmergeable label Jun 12, 2018

Merge branch 'master' into snap-form

59627b7

miq-bot removed the unmergeable label Jun 12, 2018

martinpovolny merged commit 024a68b into ManageIQ:master Jun 12, 2018

martinpovolny added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 12, 2018

Hyperkid123 deleted the snap-form branch July 10, 2018 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced ror form with react form component #3856

Replaced ror form with react form component #3856

Hyperkid123 commented Apr 26, 2018 •

edited

Hyperkid123 commented Apr 26, 2018

ohadlevy Apr 26, 2018

ohadlevy Apr 26, 2018

Hyperkid123 Apr 26, 2018

ohadlevy Apr 26, 2018

Hyperkid123 commented Apr 26, 2018 •

edited

Hyperkid123 commented Apr 26, 2018

karelhala commented Apr 27, 2018

Hyperkid123 commented Apr 30, 2018

ohadlevy commented Apr 30, 2018

ohadlevy Apr 30, 2018

karelhala May 2, 2018

Hyperkid123 commented May 3, 2018

ohadlevy May 3, 2018

Hyperkid123 May 3, 2018

ohadlevy May 7, 2018

Hyperkid123 commented May 7, 2018

ohadlevy May 7, 2018

martinpovolny May 10, 2018

martinpovolny May 10, 2018

miq-bot commented May 14, 2018

Hyperkid123 commented Jun 7, 2018

miq-bot commented Jun 7, 2018

Hyperkid123 commented Jun 7, 2018 •

edited

miq-bot commented Jun 12, 2018

miq-bot commented Jun 12, 2018

Hyperkid123 commented Jun 12, 2018

Replaced ror form with react form component #3856

Replaced ror form with react form component #3856

Conversation

Hyperkid123 commented Apr 26, 2018 • edited

Before

After

Hyperkid123 commented Apr 26, 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

Hyperkid123 commented Apr 26, 2018 • edited

Hyperkid123 commented Apr 26, 2018

karelhala commented Apr 27, 2018

Hyperkid123 commented Apr 30, 2018

ohadlevy commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 14, 2018

Hyperkid123 commented Jun 7, 2018

miq-bot commented Jun 7, 2018

Hyperkid123 commented Jun 7, 2018 • edited

miq-bot commented Jun 12, 2018

miq-bot commented Jun 12, 2018

Hyperkid123 commented Jun 12, 2018

Hyperkid123 commented Apr 26, 2018 •

edited

Hyperkid123 commented Apr 26, 2018 •

edited

Hyperkid123 commented Jun 7, 2018 •

edited