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 parameters tab on PipelineRun details page #11767

Merged
merged 4 commits into from
Jul 21, 2022

Conversation

vikram-raj
Copy link
Member

@vikram-raj vikram-raj commented Jun 28, 2022

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

Description:

  • add the parameters tab on the PipelineRun details page

Screenshots:
image

Tests e2e:
image

@vikram-raj
Copy link
Member Author

/cc @jerolimov @karthikjeeyar

@openshift-ci openshift-ci bot added component/pipelines Related to pipelines-plugin component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jun 28, 2022
Comment on lines 19 to 29
<Form>
<div className="co-m-pane__body">
<PipelineRunParameters fieldName="parameters" />
</div>
<FormFooter
handleCancel={handleCancel}
cancelLabel={t('pipelines-plugin~Cancel')}
hideSubmit
sticky
/>
</Form>
Copy link
Member

Choose a reason for hiding this comment

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

In PipelineRunParameters you define all fields as read-only. Why do we need a form and cancel button on this tab?

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial thought was the same. Removed the Form and cancel button.

Copy link
Member

@jerolimov jerolimov Jul 14, 2022

Choose a reason for hiding this comment

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

@vikram-raj But there is still a PipelineRunParametersForm.tsx and formik, this is required to fill the form fields? Then its fine for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I used Formik to fill the form fields.

@vikram-raj
Copy link
Member Author

/retest

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2022
@jerolimov
Copy link
Member

@vikram-raj Nice! Thanks for the update. Can you also add a cypress e2e test for this? 😏

@vikram-raj
Copy link
Member Author

@vikram-raj Nice! Thanks for the update. Can you also add a cypress e2e test for this? 😏

@jerolimov added cypress e2e tests.

@invincibleJai
Copy link
Member

@vikram-raj alignment looks bit off too

image

@serenamarie125
Copy link
Contributor

@beaumorley can you take a look into the disabled text inputs with PatternFly? Thanks!

@beaumorley
Copy link

It looks like the individual disabled fields have a 1 pt stroke at the bottom? If you look at the PF guideline, it is just a plain grey box. https://www.patternfly.org/v4/components/text-input Can we remove the stroke?

Also, it you take a look at a disabled on the admin side there is no stroke. Example:
image

FYI @serenamarie125
Thank you!

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2022
@vikram-raj
Copy link
Member Author

vikram-raj commented Jul 7, 2022

@beaumorley Fixed disable style.

image

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2022
@vikram-raj
Copy link
Member Author

@vikram-raj alignment looks bit off too

image

@invincibleJai Fixed the alignment issue in this PR #11799

@invincibleJai
Copy link
Member

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2022

@invincibleJai: GitHub didn't allow me to request PR reviews from the following users: check, and, add, for, once, done, do, e2d, label.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @sanketpathak do check e2d and add label for QE-verification once done

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@invincibleJai
Copy link
Member

/cc @sanketpathak

@invincibleJai
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2022
@jerolimov
Copy link
Member

/retest

@@ -231,6 +232,9 @@ export const pipelineRunDetailsPO = {
},
status: '.odc-taskrun-details__status',
},
parameters: {
form: '[data-test="pipelineRun-parameters"]',
Copy link
Contributor

@hemantsaini-7 hemantsaini-7 Jul 18, 2022

Choose a reason for hiding this comment

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

form: '[data-test="pipeline-parameters"]' instead of form: '[data-test="pipelineRun-parameters"]'

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to update the data-test after the component refactoring. Updated it now. PTAL again.
image

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
@hemantsaini-7
Copy link
Contributor

/lgtm

image

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hemantsaini-7, invincibleJai, vikram-raj

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

@beaumorley
Copy link

@vikram-raj thanks for fixing the disabled pattern. LGTM.

Copy link
Member Author

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

Propagating docs and px acks from the Epic ODC-4793
/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Jul 21, 2022
@hemantsaini-7
Copy link
Contributor

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 21, 2022
@vikram-raj
Copy link
Member Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2022

@vikram-raj: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit eee5daa into openshift:master Jul 21, 2022
@vikram-raj vikram-raj deleted the odc-6709 branch July 22, 2022 07:17
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/pipelines Related to pipelines-plugin component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants