Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom fields to support dynamic forms in formik #5789

Merged

Conversation

rohitkrai03
Copy link
Contributor

@rohitkrai03 rohitkrai03 commented Jun 19, 2020

Fixes: https://issues.redhat.com/browse/ODC-4084

Analysis / Root cause: Right now we use formik for all of our forms in dev console. Dynamic forms uses a separate framework called json-schema-form and it created based on kube resources and there actions. This wouldn't work for use because of formik forms and Helm resources not being kube native resources.

Solution Description: In order to make use of dynamic forms in the formik context, we needed to customize the already present dynamic form components and utils to support our use case for Helm install and upgrade forms. We also needed to create bridge components which bridges formik state to dynamic form state. This PR adds support for these custom components and also customizes already present components and utils.

Screen shots / Gifs for design review:
Helm Install -
Dynamic Forms Helm Install

Helm Upgrade -
Dynamic Forms Helm Upgrade

Unit Tests Added
image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@rohitkrai03
Copy link
Contributor Author

/retest

@rohitkrai03
Copy link
Contributor Author

/assign @christianvogt

@abhi-kn
Copy link
Contributor

abhi-kn commented Jun 21, 2020

/retest

@rohitkrai03
Copy link
Contributor Author

/retest

1 similar comment
@rohitkrai03
Copy link
Contributor Author

/retest

@rohitkrai03 rohitkrai03 force-pushed the formik-dynamic-form-bridge branch 2 times, most recently from aae06e7 to 91ba110 Compare June 23, 2020 16:32
@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Jun 23, 2020

/assign @TheRealJon

This PR has some updates to the dynamic form components and utils to support new use cases. It would be great to have your review for it.

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM... only a question that confuses me. Not sure you need the AsyncComponent.

Copy link
Contributor

@christianvogt christianvogt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just a small comment

@rohitkrai03 rohitkrai03 force-pushed the formik-dynamic-form-bridge branch 2 times, most recently from ef5cc0d to ca33206 Compare June 25, 2020 15:48
@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2020
@rohitkrai03
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@christianvogt
Copy link
Contributor

/lgtm cancel

I forgot to try mobile after the last change and it doesn't go to a single column anymore:
image

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 26, 2020
@christianvogt
Copy link
Contributor

/lgtm

Forgot there's another dependent PR. Get this in and fix the mobile breakpoints in #5790

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, 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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020
@openshift-merge-robot openshift-merge-robot merged commit 94c6783 into openshift:master Jun 26, 2020
@spadgett spadgett added this to the v4.6 milestone Jun 29, 2020
@rohitkrai03 rohitkrai03 deleted the formik-dynamic-form-bridge branch July 24, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/olm Related to OLM component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants