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

fix: Improve current overlay determination logic #113

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

jungpaeng
Copy link
Member

Description

Improved the overlay unmount logic to handle the current state more accurately.

Previously, it simply checked the remainingOverlays array, but now it tracks the order of opened overlays for more precise current state management.

Related Issue: Fixes #108

Changes

  • Enhanced unmount logic in reducer.ts:
    • Introduced openedOverlayOrderList to track opened overlay order
    • Implemented different current state handling based on unmounted overlay position
    • Set last overlay as current when unmounting intermediate overlays
    • Set previous overlay as current when unmounting last overlay
  • Cleaned up provider-related exports
  • Added new test cases

Motivation and Context

When multiple overlays were open simultaneously, the current state was being set incorrectly when unmounting specific overlays.

This was a critical issue that could cause confusion in user experience.
This change implements a more intuitive and predictable behavior by considering the order in which overlays were opened.

How Has This Been Tested?

  • Test for unmounting 4 overlays in various orders
  • Test for tracking current state using useCurrentOverlay hook

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have performed a self-review of my own code.
  • My code is commented, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

Further Comments

jungpaeng and others added 4 commits February 12, 2025 00:25
- Update logic to consider only currently open overlays (isOpen === true) when unmounting
- Apply different current determination logic based on unmounting overlay's position
  - Last overlay unmount: Move to previous overlay
  - Intermediate overlay unmount: Move to last overlay

Example:
Open overlays [1, 2, 3, 4]
- unmount 2 → current: 4
- unmount 4 → current: 3
- unmount 3 → current: 1
- unmount 1 → current: null

Co-authored-by: jgjgill <[email protected]>
Add test case to verify correct handling of current overlay when
removing overlays in different orders.

- Mount 4 overlays sequentially
- Unmount in order: middle(2nd), last(4th), 3rd, 1st
- Verify visibility of remaining overlays at each step
- Remove unnecessary re-export file that was duplicating exports from provider
- These exports are now directly imported from their source files
- Add test case to verify current overlay state management
- Test overlay state changes through open, close, and unmount operations
- Verify correct state tracking using useCurrentOverlay hook
@jungpaeng jungpaeng self-assigned this Feb 11, 2025
Copy link

changeset-bot bot commented Feb 11, 2025

🦋 Changeset detected

Latest commit: 059edeb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
overlay-kit Patch

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

Copy link

vercel bot commented Feb 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
overlay-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 11, 2025 5:25pm

@jungpaeng jungpaeng requested a review from Copilot February 11, 2025 15:57

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

packages/src/context/reducer.ts:88

  • [nitpick] The variable name 'openedOverlayOrderList' is ambiguous. It should be renamed to 'openOverlays' for better clarity.
const openedOverlayOrderList = state.overlayOrderList.filter(

packages/src/context/reducer.ts:91

  • [nitpick] The variable name 'targetIndexInOpenedList' is ambiguous. It should be renamed to 'targetOverlayIndex' for better clarity.
const targetIndexInOpenedList = openedOverlayOrderList.findIndex((item) => item === action.overlayId);

packages/src/context/reducer.ts:93

  • [nitpick] The comment on line 94 is verbose and could be simplified to improve readability. Consider rephrasing it to: 'If unmounting the last overlay, set the previous overlay as current. If unmounting an intermediate overlay, set the last overlay as current.'
/**
- Add isOpen condition to prevent unnecessary state updates in content-overlay-controller
- Remove debug console.log in store
- Enhance test stability by:
  - Adding waitFor assertions for async operations
  - Adding explicit isOpen checks in overlay renders
  - Adding visibility checks for overlay elements
  - Improving state transition validations
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.59%. Comparing base (9945f49) to head (059edeb).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
+ Coverage   84.24%   87.59%   +3.34%     
==========================================
  Files          13       12       -1     
  Lines         273      274       +1     
  Branches       55       60       +5     
==========================================
+ Hits          230      240      +10     
+ Misses         40       32       -8     
+ Partials        3        2       -1     
Components Coverage Δ
overlay-kit 87.59% <100.00%> (+3.34%) ⬆️

@jungpaeng jungpaeng merged commit b57d15b into main Feb 11, 2025
10 checks passed
@jungpaeng jungpaeng deleted the fix/overlay-unmount-logic branch February 11, 2025 17:28
@github-actions github-actions bot mentioned this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bugs when close & unmount multiple overlays
2 participants