-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix: Warn users when trying to delete a container #1062
Conversation
105c3b4
to
be5ddc5
Compare
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.
the behavior doesn't corresponds to the title neither the initial issue:
- the modal appears also for for steps, not only for containers
- the text message is not corresponding the proposed one in the issue. it doesn't seem to follow the recommendations from Patternfly neither https://www.patternfly.org/components/modal/design-guidelines/#confirm-a-destructive-action , like it doesn't communicate the consequences
packages/ui/src/components/Visualization/Custom/ItemDeleteStepWithConfirm.tsx
Outdated
Show resolved
Hide resolved
Thanks for the review and comment @apupier
|
abadba0
to
0e7af9d
Compare
I would slightly rephrase the dialog texts... Title: Delete multiple steps Additionally I spotted another issue...if you for instance add an empty "filter" component, the dialog pops up too regardless if the filter has children or not. |
f98550d
to
b315555
Compare
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.
I added the text changes
packages/ui/src/components/Visualization/Custom/ItemDeleteGroup.tsx
Outdated
Show resolved
Hide resolved
packages/ui/src/components/Visualization/ContextToolbar/Flows/FlowsList.tsx
Outdated
Show resolved
Hide resolved
packages/ui/src/components/Visualization/Custom/ItemDeleteStep.tsx
Outdated
Show resolved
Hide resolved
6141bd8
to
230e418
Compare
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.
Looks good, thanks for taking care of this.
There's a remaining issue that will be tackled on an upcoming PR, about deleting the from
node, which states that steps
will be removed, which it's not the case
Fixes #1042