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.

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

Merged
merged 8 commits into from
Oct 14, 2022

Conversation

varunKT001
Copy link
Contributor

Summary

Added confirmation dialog when deleting a card in calendar/galley view.

Ticket Link

Fixes #3947

@mattermod
Copy link
Contributor

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.

@Pinjasaur
Copy link
Contributor

@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 make webapp-ci. It's looking like you'll need to update a Jest snapshot, which can be done by running npm run updatesnapshot from the webapp directory. Finally, since you made some code changes that reference i18n translation strings, can you run npm run i18n-extract also from the webapp directory. Thanks!

@Rajat-Dabade
Copy link
Contributor

@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 👍

@varunKT001
Copy link
Contributor Author

@Pinjasaur @Rajat-Dabade

I have updated the Jest snapshot by running npm run updatesnapshot. I have also run the command npm run i18n-extract to update the i18n translation strings. Please review and suggest if there are any changes needed.

Thanks for helping 😄

@Rajat-Dabade
Copy link
Contributor

@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 npm run test in webapp locally if the tests are passing or not.

@varunKT001
Copy link
Contributor Author

@Pinjasaur @Rajat-Dabade

I have updated the test cases and run npm run test. There are no errors in my local setup.

image

I have also modified the code a little bit. Please review and suggest if any changes are needed.

Thank you.

@Rajat-Dabade
Copy link
Contributor

@varunKT001, I think we have a different issue now to be solved first, the CI is failing due to some linter issue in fullCalendar.tsx. Please run make webapp-ci in focalboard directory.

Also, the overlay for the Dialog in the calendar view for confirming deletion is not covering the entire screen, which it should.

Screenshot 2022-10-12 at 8 03 12 PM

@varunKT001
Copy link
Contributor Author

varunKT001 commented Oct 12, 2022

@Rajat-Dabade @Pinjasaur

I have fixed the linter errors as well as the overlay. I have run the make webapp-ci in my local setup and GitHub actions on my forked repository, and it is succeeding. Please review and suggest if any changes are needed.

image

image

Thank you.

webapp/i18n/en.json Outdated Show resolved Hide resolved
webapp/src/components/markdownEditor.test.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@Pinjasaur Pinjasaur left a 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.

Copy link
Contributor

@Rajat-Dabade Rajat-Dabade left a 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.

@Rajat-Dabade Rajat-Dabade merged commit ed3197c into mattermost:main Oct 14, 2022
@varunKT001
Copy link
Contributor Author

@Pinjasaur @Rajat-Dabade

Thank you for helping me throughout the process. I learned many new things, like the proper use of hooks like useCallback, useMemo, etc., and especially the importance of testing. It was my first contribution to such a big project.

Looking forward to contributing to more such issues 🚀

@varunKT001 varunKT001 deleted the varunKT001/issue-3947 branch October 18, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Idea: confirmation dialog when deleting a card in calendar/galley view
5 participants