Conversation
|
Thanks for the pull request, @navinkarkera! 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. |
339bc8a to
04e3ada
Compare
87cef28 to
282bdad
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2968 +/- ##
==========================================
+ Coverage 95.57% 95.59% +0.02%
==========================================
Files 1366 1375 +9
Lines 31507 31877 +370
Branches 7119 7259 +140
==========================================
+ Hits 30113 30474 +361
+ Misses 1344 1341 -3
- Partials 50 62 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
06da56f to
9fbec58
Compare
…or new design toggle stability
9fbec58 to
dbb2458
Compare
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @navinkarkera!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
NOTE: Not in the scope of this task, but it seems weird to me that "2:00" is not a valid value for Time Alloted. I think it was a bug typing it and not getting it saved.
| mutate({ | ||
| itemId: subsectionId, | ||
| sectionId: selectedContainerState?.sectionId, | ||
| isVisibleToStaffOnly: itemData.visibilityState === VisibilityTypes.STAFF_ONLY, | ||
| releaseDate: itemData.start, | ||
| graderType: itemData.format == null ? 'notgraded' : itemData.format, | ||
| dueDate: itemData.due == null ? '' : itemData.due, | ||
| isTimeLimited: itemData.isTimeLimited, | ||
| isProctoredExam: itemData.isProctoredExam, | ||
| isOnboardingExam: itemData.isOnboardingExam, | ||
| isPracticeExam: itemData.isPracticeExam, | ||
| examReviewRules: itemData.examReviewRules, | ||
| defaultTimeLimitMinutes: itemData.defaultTimeLimitMinutes, | ||
| hideAfterDue: itemData.hideAfterDue === undefined ? false : itemData.hideAfterDue, | ||
| showCorrectness: itemData.showCorrectness, | ||
| isPrereq: itemData.isPrereq, | ||
| prereqUsageKey: itemData.prereq, | ||
| prereqMinScore: defaultPrereqScore(itemData.prereqMinScore), | ||
| prereqMinCompletion: defaultPrereqScore(itemData.prereqMinCompletion), | ||
| ...variables, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Don't we allow partial updates in our API, right?
With the current approach, we override the entire config using cached data on every change, which may cause concurrency issues.
If our API is not prepared for partial updates, the change may be big for this task, so I think we should live with it.
There was a problem hiding this comment.
Yes, from what I've seen, it's always been handled that way
|
|
||
| useEffect(() => { | ||
| const element = contentRef.current; | ||
| if (!element) { return () => {}; } |
There was a problem hiding this comment.
I think we can just return undefined here instead of an empty function
| if (!element) { return () => {}; } | |
| if (!element) { return; } |
| const { setData } = useScrollState(courseKey); | ||
| const dispatch = useDispatch(); | ||
| return useMutation({ | ||
| return useMutationWithProcessingNotification({ |
| closeToast, | ||
| } = useToastContext(); | ||
| const { showToast, closeToast } = useToastContext(); | ||
| return useMutation({ |
There was a problem hiding this comment.
Can we use the useMutationWithProcessingNotification here? If not, could you please put a comment about it?
| * Wraps useMutation to add a processing notification when the mutation is initiated and removes it | ||
| * when the mutation is settled. | ||
| */ | ||
| export const useMutationWithProcessingNotification = < |
|
@navinkarkera This checkbox cannot be unchecked by clicking on it, is that expected?
|
| useEffect(() => { | ||
| if (contentRef.current) { | ||
| const { scrollHeight } = contentRef.current; | ||
| setNeedsExpansion(scrollHeight > maxHeight); | ||
| } | ||
| }, [children, maxHeight]); |
There was a problem hiding this comment.
@navinkarkera, I think this is duplicated. The above useEffect already does this

Description
Adds settings tab to section, subsection and unit sidebar in outline and unit page as described in #2794
Also, simplifies notification and unit settings in unit page using react-query, removing redux.
Useful information to include:
Screenshots:
Supporting information
Private-ref: https://tasks.opencraft.com/browse/FAL-4324Testing instructions
Other information
Leave without saving? This functionality is only working when user tries to leave the page. But changing the tab or closing the sidebar does not trigger this confirmation modal, it will require extensive changes and is not feasible with current limited time and budget.
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'