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 comment to suspend action #4147

Merged
merged 14 commits into from
Dec 4, 2023
Merged

Add comment to suspend action #4147

merged 14 commits into from
Dec 4, 2023

Conversation

ahussein3
Copy link
Contributor

Partially Closes #3546

What changed?

  • Added new Modal component to let the User who is going to suspend resources to be able to add reason for suspenstion.

Why was this change made?

  • To add the ability to add a reason for suspension.

How did you validate the change?

  • Locally testing.

ahussein3 and others added 5 commits November 22, 2023 19:45
init add comment to suspend action
09a85c8
style trials
920e4b8
@foot
Remove className and rename bodyClassName -> className
89eda3a 
- As the exisiting className doesn't really do anything with the modal
  as it using a portal and mounts the dom element elsewhere in the dom
  tree
update modal to had it's own close modal icon
6fc0e59
@ahussein3
Merge branch 'main' into AddCommentToSuspendAction
10563c6
@ahussein3 ahussein3 added the type/enhancement New feature or request label Nov 27, 2023
@ahussein3 ahussein3 marked this pull request as ready for review November 27, 2023 14:08
@opudrovs
Copy link
Contributor

JS lint is failing.

@opudrovs
Copy link
Contributor

opudrovs commented Nov 27, 2023 •
edited
Loading

There is a typo in the suspend modal title:

Screenshot 2023-11-27 at 16 31 06

@opudrovs
Copy link
Contributor

❔ Probably expected behavior, just to make sure: is it OK that

  • a modal is displayed and a suspend comment is added to the toggle suspend request when suspending an object (for example, an automation) from the object's details pages (with the SyncActions component to which the new SuspendMessageModal component was added)
  • but not when suspending (possibly multiple) objects with the CheckboxActions component from the objects' list page?

opudrovs
opudrovs approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@opudrovs opudrovs 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 it locally, can see the modal when suspending objects from either details pages or object list.

foot
foot reviewed Nov 30, 2023
View reviewed changes
ui/components/Sync/SuspendMessageModal.tsx Outdated Show resolved Hide resolved
ui/components/Sync/SuspendMessageModal.tsx Outdated Show resolved Hide resolved
ui/components/Sync/SuspendMessageModal.tsx Outdated Show resolved Hide resolved
ui/components/Sync/SuspendMessageModal.tsx Show resolved Hide resolved
ui/components/Sync/SuspendMessageModal.tsx Show resolved Hide resolved
@ahussein3 ahussein3 requested a review from foot December 2, 2023 12:32
@ahussein3 ahussein3 merged commit 76ffb12 into main Dec 4, 2023
15 checks passed
@ahussein3 ahussein3 deleted the AddCommentToSuspendAction branch December 4, 2023 17:24
@weave-gitops-bot weave-gitops-bot mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opudrovs opudrovs opudrovs approved these changes

@foot foot foot approved these changes

Assignees
No one assigned
Labels
type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit log of suspensions
3 participants
@ahussein3 @opudrovs @foot