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.

Sign up for GitHub

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

Jump to bottom

Add YAML switcher to pipeline builder form #7028

Merged
merged 1 commit into from Nov 12, 2020
Merged

Add YAML switcher to pipeline builder form #7028

merged 1 commit into from Nov 12, 2020

Conversation

rottencandy
Copy link
Contributor

@rottencandy rottencandy commented Oct 29, 2020 •
edited

Story:
https://issues.redhat.com/browse/ODC-3662

Description:
Add a YAML switcher allowing the ability to dynamically switch between YAML view and form view, including persisting form data including errors in both views.

Screen shots / Gifs for design review:
tmp0
tmp1
tmp
tmp2

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

cc: @openshift/team-devconsole-ux @andrewballantyne @karthikjeeyar

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2020
@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console component/shared Related to console-shared labels Oct 29, 2020
@rottencandy rottencandy changed the title [wip]Add YAML switcher to pipeline builder form Add YAML switcher to pipeline builder form Oct 29, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2020
@karthikjeeyar
Copy link
Contributor

@rottencandy On clicking Create button in yaml view does nothing for me, Please confirm if it is working for you. Reset button works fine.
image

@rottencandy
Copy link
Contributor Author

@rottencandy On clicking Create button in yaml view does nothing for me, Please confirm if it is working for you. Reset button works fine.

@karthikjeeyar updated the validation schema. It should work properly now.

@rottencandy
Copy link
Contributor Author

/retest

@andrewballantyne
Copy link
Contributor

/assign

andrewballantyne
andrewballantyne suggested changes Nov 3, 2020
View reviewed changes
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.

First pass to clean things up. Will try to review more tomorrow.

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/validation-utils.ts Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/types.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/types.ts Outdated Show resolved Hide resolved
...kages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderVisualization.tsx Outdated Show resolved Hide resolved
@rottencandy rottencandy force-pushed the pipelineyaml branch 2 times, most recently from 25f2d46 to b186c03 Compare November 4, 2020 07:39
andrewballantyne
andrewballantyne suggested changes Nov 5, 2020
View reviewed changes
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts Outdated Show resolved Hide resolved
...packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderFormEditor.tsx Outdated
const { status } = useFormikContext<PipelineBuilderFormikValues>();

return (
<StackItem isFilled className="odc-pipeline-builder-form__content">
Copy link
Contributor

Choose a reason for hiding this comment

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

So when adding wrapping containers, you have to be careful what the elements you're wrapping are.

This is a StackItem, which should be inside a Stack - but if you look at the rendered output, the form gets in the way:
Screen Shot 2020-11-05 at 5 30 PM (marked up)

Suggested change
<StackItem isFilled className="odc-pipeline-builder-form__content">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, makes sense. Removed. I wonder if it's okay to have something that is not a StackItem directly under Stack.

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx Show resolved Hide resolved
frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx Show resolved Hide resolved
andrewballantyne
andrewballantyne approved these changes Nov 10, 2020
View reviewed changes
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.

Enums in types are not the end of the world, but we should try to keep the items together.

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/types.ts Outdated
Comment on lines 46 to 59
export enum UpdateOperationType {
ADD_LIST_TASK,
CONVERT_LIST_TO_TASK,
UPDATE_TASK,
REMOVE_TASK,
DELETE_LIST_TASK,
FIX_INVALID_LIST_TASK,
}

export enum TaskErrorType {
MISSING_REQUIRED_PARAMS = 'missingParams',
MISSING_NAME = 'nameMissing',
MISSING_RESOURCES = 'missingResources',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So types can have circular references. TypeScript handles this better than JS imports of code. You should be able to return these enums to the other file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter didn't let me commit, which is why I did the suffling. I've disabled the warnings now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, damn, that was not necessary. I didn't see the lint error on my side. I didn't try to commit naturally haha... and I didn't run the linter. Must be something my IDE is not picking up on. 🤦 This is worse actually then what you originally did... but I think we can leave it. I'll LGTM this PR and just get it merged. We can clean up once it's in it's own package.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2020
@andrewballantyne
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, rottencandy

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-merge-robot openshift-merge-robot merged commit 871a2ee into openshift:master Nov 12, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 16, 2020
@rottencandy rottencandy deleted the pipelineyaml branch December 7, 2020 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewballantyne andrewballantyne andrewballantyne approved these changes

@jhadvig jhadvig Awaiting requested review from jhadvig

@nemesis09 nemesis09 Awaiting requested review from nemesis09

Assignees

@andrewballantyne andrewballantyne

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rottencandy @karthikjeeyar @andrewballantyne @openshift-ci-robot @spadgett @openshift-merge-robot