-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix: Enhance overlay reducer state management #120
Conversation
- Add checks to prevent unnecessary state updates for non-existent or already open/closed overlays - Introduce current overlay tracking when opening/closing/removing overlays - Handle edge cases like closing all overlays when no overlays exist
🦋 Changeset detectedLatest commit: 5073ef2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
==========================================
- Coverage 96.72% 95.86% -0.87%
==========================================
Files 12 12
Lines 275 290 +15
Branches 67 73 +6
==========================================
+ Hits 266 278 +12
- Misses 7 10 +3
Partials 2 2
|
packages/src/context/reducer.ts
Outdated
return { | ||
...state, | ||
current: action.overlayId, |
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.
When the following code is executed:
overlay.open(() => ..., { overlayId: 'A' });
overlay.open(() => ..., { overlayId: 'B' });
overlay.open(() => ..., { overlayId: 'C' });
The dispatch sequence proceeds as follows:
{ type: 'ADD', overlayId: 'A' }
{ type: 'ADD', overlayId: 'B' }
{ type: 'ADD', overlayId: 'C' }
{ type: 'OPEN', overlayId: 'A' }
{ type: 'OPEN', overlayId: 'B' }
{ type: 'OPEN', overlayId: 'C' }
During this process, the current
value changes in the order of A -> B -> C -> A -> B -> C
, which feels unnatural.
Was there a specific reason for updating the current
value in the OPEN
action?
If this approach is applied, it seems better to remove the current
value update from the ADD
action.
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.
Didn't realize dispatch of current overlay happens outside reducer. Removed at: f472720
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.
Description
Related Issue: Fixes N/A
Changes
isOpen === false
clause fromContentOverlayController
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist
Further Comments