-
Notifications
You must be signed in to change notification settings - Fork 11.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
chore: Discussion message action meteor removal #33962
Conversation
Looks like this PR is ready to merge! 🎉 |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #33962 +/- ##
========================================
Coverage 75.73% 75.73%
========================================
Files 507 507
Lines 21799 21799
Branches 5343 5343
========================================
Hits 16509 16509
Misses 4648 4648
Partials 642 642
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
apps/meteor/client/components/message/toolbar/useNewDiscussionMessageAction.tsx
Show resolved
Hide resolved
Could you also add some tests to cover this scenario? |
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 think the proper fix for the issue should be use the isLoading
from the createDiscussionMutation
to set the loading
while clicking in the create button. The code you did seems a good refactor so we could handle separately under the initiative we want to start.
Regarding tests, we already have a e2e test for this. I think it's just missing the assertion to guarantee, that only one discussion was created
Split the fix and the chore, fix moved to #33985 |
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!
Proposed changes (including videos or screenshots)
Moves the discussion message action to a hook
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1348