-
Notifications
You must be signed in to change notification settings - Fork 667
Add Remove Trigger #5012
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 Remove Trigger #5012
Conversation
divyanshiGupta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewballantyne apart from your last commit, other commits are reorganizing/refactoring code or they add or change any functionalities?
...end/packages/dev-console/src/components/pipelines/modals/common/PipelineParameterSection.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/pipelines/modals/triggers/remove-utils.ts
Outdated
Show resolved
Hide resolved
Apologizes, been reviewing and haven't had a chance to refactor (actually doing that right now). Everything before the last commit is stacking PRs. The last commit is the only one that matters for the content of this PR. |
2bffaf5 to
a39b8f6
Compare
|
@divyanshiGupta I have made this PR a tiny bit smaller 🙂 |
979d585 to
918f1da
Compare
Its like you removed 80% of the code. 😜 |
divyanshiGupta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewballantyne the code looks good to me. Need to try to it locally once. Also please resolve the conflicts.
...nd/packages/dev-console/src/components/pipelines/modals/triggers/TriggerTemplateSelector.tsx
Outdated
Show resolved
Hide resolved
918f1da to
8d9f55f
Compare
|
@andrewballantyne verified removing triggers, it works fine.
|
frontend/packages/dev-console/src/components/pipelines/modals/common/ModalStructure.tsx
Outdated
Show resolved
Hide resolved
Absolutely. Unless there is a critical issue, no additions are happening on the current Pipeline PRs. We'll cover it after FF. I'll log a bug to track (https://issues.redhat.com/browse/ODC-3589). |
Tried it locally, works fine. Will add lgtm once the remaining comments are resolved. |
8d9f55f to
3cad446
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, karthikjeeyar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Looks good. |
|
/retest |
That's a first. /test frontend |
|
/retest |
1 similar comment
|
/retest |
|
/kind feature |

Fixes:
https://issues.redhat.com/browse/ODC-3349
Analysis / Root cause:
Need a way to remove the triggers we add.
Solution Description:
Remove trigger modal.
Screen shots / Gifs for design review:
@openshift/team-devconsole-ux
Empty state seems lame... Probably should find a way not to show the modal... but it requires fetching to find out that information... 🤔

I added a minor gap slightly smaller than a single line message to hack our way around PatternFly and their dropdown/modal issue:

Selected and ready to remove:

Unit test coverage report:
Test setup:
Browser conformance: