-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[GH-3947]: Added confirmation dialog when deleting a card in calendar/galley view #3996
[GH-3947]: Added confirmation dialog when deleting a card in calendar/galley view #3996
Conversation
Hello @varunKT001, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
@varunKT001 thanks for your contribution! PR looks to in a good spot, just appears to need a bit of tweaking to pass CI. To simulate the webapp CI runs locally you can use |
@varunKT001, the changes look good to me as well. I do agree with @Pinjasaur suggestion and once you are done with the suggested modification, which I guess will fix all the CI issues, we can merge it. Thanks for the contribution. Nice work 👍 |
I have updated the Jest snapshot by running Thanks for helping 😄 |
@varunKT001, thanks for implementing the suggested changes. It seems to be we have CI failing again, I suspect it is due to the additional confirmation Dialog box for deleting the card (changes in this PR). You can just update the test case and try running |
I have updated the test cases and run I have also modified the code a little bit. Please review and suggest if any changes are needed. Thank you. |
@varunKT001, I think we have a different issue now to be solved first, the CI is failing due to some linter issue in Also, the overlay for the Dialog in the |
I have fixed the linter errors as well as the overlay. I have run the Thank you. |
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.
LGTM @varunKT001. Thanks for your contribution & bearing with us on the i18n-extract
issue. Hopefully it'll be fixed soon so it doesn't happen in the future. 🙂
@Rajat-Dabade looked to have a question or 2 left, so I'll defer there.
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.
LGTM, nice work @varunKT001 🚀. Thanks for the contribution, really appreciate your work.
Thank you for helping me throughout the process. I learned many new things, like the proper use of hooks like Looking forward to contributing to more such issues 🚀 |
Summary
Added confirmation dialog when deleting a card in calendar/galley view.
Ticket Link
Fixes #3947