refactor: Migrate processingNotification from redux store to use the functions in ToastContext#2917
Conversation
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
processingNotification from redux store to use the functions in ToastContestprocessingNotification from redux store to use the functions in ToastContext
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2917 +/- ##
========================================
Coverage 95.51% 95.51%
========================================
Files 1329 1327 -2
Lines 30544 30557 +13
Branches 6941 6697 -244
========================================
+ Hits 29173 29188 +15
- Misses 1303 1313 +10
+ Partials 68 56 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // TODO, We can convert this to a queue of messages, | ||
| // see: https://github.com/open-craft/frontend-app-course-authoring/pull/38#discussion_r1638990647 |
There was a problem hiding this comment.
Are you interested in doing this now? I'm a little worried about all the closeToast() method calls potentially hiding error messages if we're using this context for so many different things.
e.g. Export says "show toast", then 1s later something else shows an error toast, then export calls "close toast" and the user never sees the middle error.
There was a problem hiding this comment.
Yes, I can do it in a separate PR
src/generic/toast-context/index.tsx
Outdated
| closeToast: () => {}, | ||
| }); | ||
|
|
||
| // Module-level references to showToast and closeToast, kept in sync by ToastProvider. |
There was a problem hiding this comment.
We could just make these global variables then, instead of using a context provider.
There was a problem hiding this comment.
@bradenmacdonald In the comments, I forgot to mention that this is, in theory, temporary. These functions are used in thunks, which will be migrated to React Query. In the new functions, it will be possible to call useToastContext(). Furthermore, for more complex functions like the message queue, the context would be better. What do you think?
There was a problem hiding this comment.
@ChrisChV OK, please mention that in a comment in the code.
src/course-outline/CourseOutline.tsx
Outdated
| || isSectionHighlightsUpdatePending | ||
| || isDuplicatingItem | ||
| || isPasting) { | ||
| showToast(NOTIFICATION_MESSAGES.saving); |
There was a problem hiding this comment.
This shows the toast, but it doesn't hide it when the processing is done, and I think the code before did that?
For example, if I add a new section to the course, it says "Saving" for 1 second and then I see the new section appear, but it still says "Saving" for a few more seconds, even though the new section is already fully created.
The unit page is working fine though; the problem is only on the outline page.
There was a problem hiding this comment.
It's true. I refactored that code in dc730de
Description
processingNotificationfrom redux store to use the functions inToastContext.Supporting information
Testing instructions
Certificates
Settings > Certificates.Course Outline
Course Unit
Course Updates
Content > Course updates.Group Configuration
Settings > Group Configurations.Textbooks
Content > Pages & resources > TextbooksOther information
N/A
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'