Converted existing Region edit form to DDF/React/API form #6978

h-kataria · 2020-04-11T03:36:25Z

h-kataria · 2020-04-11T03:41:38Z

@skateman @himdel my first stab at DDF/REACT/API form, picked a simple one to get started.

skateman

I'd rather use hooks instead of a class component, all the required stuff for that is available here: https://reactjs.org/docs/hooks-reference.html

skateman · 2020-04-11T07:35:49Z

app/javascript/components/region-form/region-form.schema.js

@@ -0,0 +1,20 @@
+import {validatorTypes} from "@data-driven-forms/react-form-renderer";


Missing spaces around validatorTypes

skateman · 2020-04-11T07:36:25Z

app/javascript/components/region-form/index.jsx

+
+RegionForm.propTypes = {
+  maxDescLen: PropTypes.number.isRequired,
+  regionFormId: PropTypes.number.isRequired,


If this is just an id field, why not name it id or regionId if we want to be precise?

skateman · 2020-04-11T07:36:49Z

app/javascript/components/region-form/index.jsx

+import { Grid } from 'patternfly-react';
+import createSchema from './region-form.schema';
+import MiqFormRenderer from '../../forms/data-driven-form';
+import {API} from '../../http_api';


Missing spaces around API

skateman · 2020-04-11T07:36:56Z

app/javascript/components/region-form/index.jsx

@@ -0,0 +1,71 @@
+import React, {Component, useState} from 'react';


Missing spaces

skateman · 2020-04-11T07:37:49Z

app/javascript/components/region-form/index.jsx

+    };
+  }
+
+  componentDidMount() {


Please use the useEffect hook instead componentDidMount, then you can simplify it to a function.

skateman · 2020-04-11T07:38:33Z

app/javascript/components/region-form/index.jsx

+  constructor(props) {
+    super(props);
+    this.state = {
+      schema: createSchema(props.maxDescLen),


Do we really need the schema in the state?

skateman · 2020-04-11T07:39:36Z

app/javascript/components/region-form/index.jsx

+import handleFailure from "../../helpers/handle-failure";
+import miqRedirectBack from "../../helpers/miq-redirect-back";
+
+class RegionForm extends Component {


Instead of a class component could you make a function component? Then you can eliminate the constructor function and have just useState below.

skateman · 2020-04-11T07:40:39Z

app/javascript/components/region-form/index.jsx

+  }
+
+  onCancel = () => {
+    const message = sprintf(__('Edit of Region was cancelled by the user'));


Do you really need the sprintf here?

skateman · 2020-04-11T07:41:19Z

app/javascript/components/region-form/index.jsx

+    return request().then(() => miqRedirectBack(message, 'success', submitUrl)).catch(miqSparkleOff);
+  }
+
+  render() {


If you're using a function component, you don't need this render(), just simply return with the markup from the function.

h-kataria · 2020-04-11T16:25:35Z

@skateman please re-review, made suggested changes

skateman · 2020-04-12T06:27:44Z

app/javascript/components/region-form/index.jsx

+
+const RegionForm = ({ id, maxDescLen }) => {
+  const [initialValues, setInitialValues] = useState({});
+  const [isLoading, setIsLoading] = useState(!!id);


You don't really need this second one, you can just test against the initialValues in the rendering.

Also if you can always use a single state object like this:

const [{ description, isLoading }, setState] = useState({});

But in this case you don't really need it.

skateman · 2020-04-12T06:28:12Z

app/javascript/components/region-form/index.jsx

+  const [isLoading, setIsLoading] = useState(!!id);
+
+  useEffect(() => {
+    if (id) {


Do you really need this? Can the id be not set?

skateman · 2020-04-12T06:28:21Z

app/javascript/components/region-form/index.jsx

+               }) => setInitialValues({
+          ...data,
+        }))
+        .then(() => setIsLoading(false))


You can skip this step

skateman · 2020-04-12T06:29:15Z

app/javascript/components/region-form/index.jsx

+      miqSparkleOn();
+      API.get(`/api/regions/${id}?attributes=description`)
+        .then(({
+                 ...data


You can just simply use data without the {...}

Thinking this through, maybe you just need the description field and nothing more, so you don't even need the whole initialValues, just a { description }

skateman · 2020-04-12T06:29:40Z

app/javascript/components/region-form/index.jsx

+        .then(({
+                 ...data
+               }) => setInitialValues({
+          ...data,


Probably here too, no need for copying, especially twice.

skateman · 2020-04-12T06:29:51Z

app/javascript/components/region-form/index.jsx

+        .then(() => setIsLoading(false))
+        .then(miqSparkleOff);
+    }
+  }, []);


Suggested change

}, []);

}, [id]);

skateman · 2020-04-12T06:30:16Z

app/javascript/components/region-form/index.jsx

+    return request().then(() => miqRedirectBack(message, 'success', submitUrl)).catch(miqSparkleOff);
+  };
+
+  if (isLoading) {


Suggested change

if (isLoading) {

if (!initialValues) {

skateman · 2020-04-12T06:33:45Z

app/javascript/components/region-form/index.jsx

+    miqSparkleOn();
+    const message = __('Region was saved');
+    const submitUrl = `/ops/explorer/${id}?button=save`;
+    const request = () => API.post(`/api/regions/${id}`, {


No need for the request variable allocation here, you can chain like:

return API.post().then().catch()

as you're not really using the request afterwards.

h-kataria · 2020-04-12T14:14:22Z

@skateman made suggested changes.

skateman

It's Easter, we shouldn't be working today

skateman · 2020-04-12T20:10:45Z

app/javascript/components/region-form/index.jsx

+import miqRedirectBack from "../../helpers/miq-redirect-back";
+
+const RegionForm = ({ id, maxDescLen }) => {
+  const [description, setInitialValues] = useState({});


Suggested change

const [description, setInitialValues] = useState({});

const [description, setDescription] = useState();

skateman · 2020-04-12T20:11:22Z

app/javascript/components/region-form/index.jsx

+  useEffect(() => {
+    miqSparkleOn();
+    API.get(`/api/regions/${id}?attributes=description`)
+      .then(({ description }) => setInitialValues({ description }))


Suggested change

.then(({ description }) => setInitialValues({ description }))

.then(({ description }) => setDescription(description))

skateman · 2020-04-12T20:11:47Z

app/javascript/components/region-form/index.jsx

+    const message = __('Region was saved');
+    const submitUrl = `/ops/explorer/${id}?button=save`;
+
+    return API.post(`/api/regions/${id}`, {


I don't think we need to return here at all

skateman · 2020-04-12T20:12:03Z

app/javascript/components/region-form/index.jsx

+  return (
+    <Grid fluid style={{ paddingTop: 20 }}>
+      <MiqFormRenderer
+        initialValues={description}


Suggested change

initialValues={description}

initialValues={{ description }}

not sure why we need those extra curlies here, those populate text field with value [object Object] instead of an actual value of the description.

It's about semantics, the initialValues should contain the description not be it. The extra curly here basically does:

initialValues = { description: description }

skateman · 2020-04-12T20:13:24Z

app/helpers/view_helper.rb

  MAX_NAME_LEN = 255
  MAX_DESC_LEN = 255
  MAX_HOSTNAME_LEN = 255
+  MAX_DESC_LEN_SHORT = 50


Is this name descriptive enough? Out of context I wouldn't really understand what this is 😕

well to me that made sense, but if we are nitpicking all variable names here, i would go with MAX_LEN_SMALL_TEXT_FIELD makes sense that is the length of a small text field in the form

I just thought that you'd want to use this constant on this single form, but now I understand. It can be either way I guess.

skateman · 2020-04-13T07:37:26Z

app/javascript/components/region-form/index.jsx

+  return (
+    <Grid fluid style={{ paddingTop: 20 }}>
+      <MiqFormRenderer
+        initialValues={description}


It's about semantics, the initialValues should contain the description not be it. The extra curly here basically does:

initialValues = { description: description }

skateman · 2020-04-13T07:37:41Z

app/javascript/components/region-form/index.jsx

+  useEffect(() => {
+    miqSparkleOn();
+    API.get(`/api/regions/${id}?attributes=description`)
+      .then(({ description }) => setDescription({ description }))


description = description instead of description = { description: description }

Suggested change

.then(({ description }) => setDescription({ description }))

.then(({ description }) => setDescription(description))

skateman · 2020-04-13T07:37:49Z

app/javascript/components/region-form/index.jsx

+import miqRedirectBack from "../../helpers/miq-redirect-back";
+
+const RegionForm = ({ id, maxDescLen }) => {
+  const [description, setDescription] = useState({});


The description should be undefined and not an empty object when mounting the component:

Suggested change

const [description, setDescription] = useState({});

const [description, setDescription] = useState();

app/javascript/components/region-form/index.jsx

Fixes ManageIQ#6977

skateman · 2020-04-13T21:44:52Z

Code looks good now, I'll test it tomorrow 👍

skateman

himdel · 2020-04-14T12:10:38Z

app/javascript/components/region-form/index.jsx

+
+RegionForm.propTypes = {
+  maxDescLen: PropTypes.number.isRequired,
+  id: PropTypes.number.isRequired,


ids must be strings in js (only database ids, because of the high region precision issue)

himdel · 2020-04-14T12:13:15Z

app/javascript/spec/region-form/region-form.spec.js

+  const flashLaterSpy = jest.spyOn(window, 'miqFlashLater');
+  beforeEach(() => {
+    initialProps = {
+      id: 123,


himdel · 2020-04-14T12:13:34Z

app/views/ops/_settings_details_tab.html.haml

-                :force_cancel_button => true,
-                :ajax_buttons => true})
-
+      = react('RegionForm', :maxDescLen => ViewHelper::MAX_LEN_SMALL_TEXT_FIELD, :id => region.id)


region.id.to_s

himdel · 2020-04-14T12:16:42Z

Looks great :)

The id thing is the only thing I'd change :)

Fixes ManageIQ#6977

miq-bot · 2020-04-14T13:05:56Z

Checked commits h-kataria/manageiq-ui-classic@479e749~...a916e3f with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 👍

h-kataria requested review from himdel and skateman April 11, 2020 03:36

h-kataria assigned himdel Apr 11, 2020

h-kataria force-pushed the region_edit_conversion branch from bd579bf to 6e0b692 Compare April 11, 2020 04:14

skateman suggested changes Apr 11, 2020

View reviewed changes

h-kataria force-pushed the region_edit_conversion branch from 6e0b692 to 3d9f9f8 Compare April 11, 2020 16:24

h-kataria force-pushed the region_edit_conversion branch from 3d9f9f8 to 6354207 Compare April 11, 2020 16:32

skateman suggested changes Apr 12, 2020

View reviewed changes

h-kataria force-pushed the region_edit_conversion branch from 6354207 to 3356c02 Compare April 12, 2020 14:03

skateman suggested changes Apr 12, 2020

View reviewed changes

h-kataria force-pushed the region_edit_conversion branch from 3356c02 to 8cbb2ff Compare April 12, 2020 22:30

skateman suggested changes Apr 13, 2020

View reviewed changes

Converted existing Region edit form to DDF/React/API form

479e749

Fixes ManageIQ#6977

h-kataria force-pushed the region_edit_conversion branch 2 times, most recently from d135cdf to d56d19c Compare April 13, 2020 20:29

skateman approved these changes Apr 14, 2020

View reviewed changes

himdel reviewed Apr 14, 2020

View reviewed changes

Added spec test for the new React form

a916e3f

Fixes ManageIQ#6977

h-kataria force-pushed the region_edit_conversion branch from d56d19c to a916e3f Compare April 14, 2020 13:03

himdel merged commit f829ace into ManageIQ:master Apr 14, 2020

himdel added the enhancement label Apr 14, 2020

h-kataria deleted the region_edit_conversion branch April 14, 2020 14:22

asirvadAbrahamVarghese mentioned this pull request Aug 4, 2025

Create cypress tests #9525

Open

84 tasks

		@@ -0,0 +1,20 @@
		import {validatorTypes} from "@data-driven-forms/react-form-renderer";

		@@ -0,0 +1,71 @@
		import React, {Component, useState} from 'react';

	const [description, setInitialValues] = useState({});
	const [description, setDescription] = useState();

	.then(({ description }) => setInitialValues({ description }))
	.then(({ description }) => setDescription(description))

Converted existing Region edit form to DDF/React/API form #6978

Converted existing Region edit form to DDF/React/API form #6978

Uh oh!

Conversation

h-kataria commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-kataria commented Apr 11, 2020

Uh oh!

skateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-kataria commented Apr 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-kataria commented Apr 12, 2020

Uh oh!

skateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

h-kataria commented Apr 11, 2020 •

edited

Loading

himdel Apr 14, 2020 •

edited

Loading