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

added support for cancel and restart in export app #9916

Merged
merged 1 commit into from Aug 30, 2021
Merged

added support for cancel and restart in export app #9916

merged 1 commit into from Aug 30, 2021

Conversation

invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Aug 30, 2021 •
edited

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

Solution Description:
Added support for cancel and restart in the export app.

Screen shots / Gifs for design review:

image

cc @openshift/team-devconsole-ux

Unit test coverage report:

image

image

Test setup:

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added component/topology Related to topology kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2021
sahil143
sahil143 suggested changes Aug 30, 2021
View reviewed changes
frontend/packages/topology/src/components/export-app/export-app-modal.tsx Outdated Show resolved Hide resolved
frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx
Comment on lines +13 to +16
startTime?: string;
onCancelExport?: () => Promise<boolean>;
onRestartExport?: () => Promise<boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these props optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

as there is a scenario when export is triggered but res data is not present so her we just that export is started, to observe this double click on export app btn

frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx Outdated Show resolved Hide resolved
frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx

const onCancel = async () => {
try {
await onCancelExport();
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is an optional prop

Suggested change
await onCancelExport();
await onCancelExport?.();

Copy link
Member Author

Choose a reason for hiding this comment

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

onCancel will be invoked only if onCancelExport is present as btn is rendered then

frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx Show resolved Hide resolved
frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx Outdated Show resolved Hide resolved
frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx Outdated Show resolved Hide resolved
sahil143
sahil143 approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

/lgtm

tested locally. works as expected

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021
@sahil143
Copy link
Contributor

/hold

cc @christianvogt

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2021
@christianvogt
Copy link
Contributor

fyi @serenamarie125

serenamarie125
serenamarie125 approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for getting this in so quickly!

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021
@invincibleJai
Copy link
Member Author

/label docs-approved
/label px-approved

added based on ack on the associated epic from px and docs

@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 Aug 30, 2021
@christianvogt
Copy link
Contributor

/lgtm
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, sahil143, serenamarie125

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

@sahil143
Copy link
Contributor

/label qe-approved

tested locally. works as expected

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit efc4bb2 into openshift:master Aug 30, 2021
@spadgett spadgett added this to the v4.9 milestone Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sahil143 sahil143 sahil143 approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@jerolimov jerolimov Awaiting requested review from jerolimov

@rottencandy rottencandy Awaiting requested review from rottencandy

Assignees

@sahil143 sahil143

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/topology Related to topology 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
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@invincibleJai @sahil143 @christianvogt @serenamarie125 @spadgett @openshift-merge-robot