Move Formik fields and form helpers from @console/dev-console to @console/shared #3516

jtomasek · 2019-11-21T13:07:01Z

~~Depends on #3469~~

christianvogt · 2019-11-21T20:07:44Z

cc @rohitkrai03

andrewballantyne

@rohitkrai03 should be back on Monday... I didn't see this PR until today.

/assign

andrewballantyne · 2019-11-29T21:12:39Z

frontend/packages/console-shared/src/components/formik-fields/ResourceDropdownField.tsx

-import ResourceDropdown from '../dropdown/ResourceDropdown';
+import ResourceDropdown from '@console/dev-console/src/components/dropdown/ResourceDropdown';


This will be a problem. shared cannot depend on dev-console. The ResourceDropdown doesn't appear to have any dependencies on dev-console, so I suggest moving that over to shared as well.

I am happy to move the ResourceDropdown as well. Just a question: Are the ResourceDropdown, ResourceDropdownField and ResourceLimitField generic enough? Can we expect them to be reused by other packages? If those are fairly specific to dev-console, we should keep them there instead.

Never mind, I already see that it is being used by noobaa-storage-plugin already, so answer is yes.

andrewballantyne · 2019-11-29T22:33:15Z

frontend/packages/console-shared/src/components/formik-fields/SwitchField.tsx

-import { getFieldId } from '@console/dev-console/src/components/formik-fields/field-utils';
-import { CheckboxFieldProps } from '@console/dev-console/src/components/formik-fields/field-types';
+import { getCheckboxFieldId } from './field-utils';
+import { CheckboxFieldProps } from './field-types';

 const SwitchField: React.FC<CheckboxFieldProps> = ({


Yikes, had no idea SwitchField was so similar to CheckboxField... we need to deduplicate this code using some sort of HOC or something. The logic of both of these components is near identical.

@christianvogt @rohitkrai03 I recommend we look to clean this up after this PR as to keep everything from crossing wires.

Interesting, this didn't come from dev-console. I just assumed all fields were in our package. Either way, this should probably be cleaned up, it's near identical code.

@andrewballantyne I think we can merge both of these components and control the component it renders internally using a prop. Something like

<CheckboxField as="switch" {...prop} />

This is inline with how formik handles rendering form fields with different components. We could make checkbox as default variant as well.

I like keeping SwitchField as a separate component as it is easier for devs to know it exists. How about this? jtomasek@9f2a976
I've used render props pattern and refactored common code into CheckBoxFieldBase component. It would be IMHO better to keep that change in a separate PR.

Definitely agree, it should be another PR, this one should track the refactor (it's a big enough change to be done on it's own).

FWIW, I lean the render prop way that @jtomasek suggested. I had envisioned more of a function composition where the Component would be passed in and invoked with JSX inside a nested function -- but it seems our hook lint doesn't like that idea. The render prop is a decent alternative imo.

andrewballantyne · 2019-11-29T22:38:00Z

frontend/packages/dev-console/src/components/import/advanced/BuildConfigSection.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { CheckboxField, EnvironmentField } from '../../formik-fields';
+import { CheckboxField, EnvironmentField } from '@console/shared/src/components/formik-fields';


Recommend we change all of these to be @console/shared and export all the formik fields out of the console-shared/src/components/index.ts file, following the other styles of what are done there. I don't think there is any need to know the path to where the formik fields are located when it's in a common shared package.

There are quite a few of the path updates, so I won't spam comments :)

Done. I've re-exported the components, utils and types from the index and updated the imports.

andrewballantyne · 2019-11-29T22:39:53Z

frontend/packages/dev-console/src/components/import/image-search/ImageStream.tsx

@@ -18,7 +18,7 @@ import {
  getImageStreamTags,
 } from '../../../utils/imagestream-utils';
 import './ImageStream.scss';
-import ResourceDropdownField from '../../formik-fields/ResourceDropdownField';
+import ResourceDropdownField from '../../../../../console-shared/src/components/formik-fields/ResourceDropdownField';


Suggested change

import ResourceDropdownField from '../../../../../console-shared/src/components/formik-fields/ResourceDropdownField';

import { ResourceDropdownField } from '@console/shared';

After the above change is done.

andrewballantyne · 2019-11-29T22:44:22Z

frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingModal.tsx

@@ -10,7 +10,7 @@ import {
  MultiColumnField,
  InputField,
  DropdownField,
-} from '@console/dev-console/src/components/formik-fields';
+} from '@console/shared/src/components/formik-fields';


Oh, this is a great change. Good catch! knative-plugin shouldn't rely on dev-console.

andrewballantyne · 2019-11-29T22:47:33Z

This is a good update, thanks for taking this on @jtomasek

jtomasek · 2019-12-02T17:48:22Z

frontend/packages/console-shared/src/components/formik-fields/index.ts

+export { default as SwitchField } from './SwitchField';
+export { default as TextAreaField } from './TextAreaField';
+export * from './field-utils';
+export * from './field-types';


I am slightly concerned if we really want to re-export these. It allows to use '@console/shared' for imports but we could hit naming collisions/confusion as the @console/shared package grows. Should we re-export everything which is used outside the shared package? Should we re-export only selected components and utils? Should we split shared package into multiple packages (e.g. @console/shared-forms) to avoid potential naming conflicts?

We could definitely create more packages for isolated use, but we need to be very clear with who is dependant on who otherwise we run the risk of circular references. I wonder if we could just export at a deeper level... something like @console/shared/src/formik or something...

@christianvogt @spadgett thoughts?

Perhaps we circle back to this - you may have a growing issue with rebases as time goes on. Recommend we get this in and circle back to address the export.

rohitkrai03 · 2019-12-03T12:46:47Z

/assign

* add hancleCancel function to allow for usecases where form reset and cancel is needed * add ability to specify custom button labels * make successMessage prop optional

to '@console/shared'

jtomasek · 2019-12-03T15:35:33Z

Rebased + removed dependency on #3469

andrewballantyne · 2019-12-03T15:42:28Z

Need an approver to apply the other label. @christianvogt @spadgett

christianvogt · 2019-12-03T16:48:48Z

/approve

openshift-ci-robot · 2019-12-03T16:49:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jtomasek, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewballantyne · 2019-12-03T17:06:02Z

Logged tickets to cover these.

Investigate a better export methodology for isolated parts of the shared package... such as Formik's Components / Utilities

https://jira.coreos.com/browse/ODC-2468

Address the duplicate nature of Switch Component and Checkbox Component now that they both live under shared

https://jira.coreos.com/browse/ODC-2469

openshift-bot · 2019-12-03T17:14:37Z

/retest