Feature/matb/fix floor selector expansion on small screens#669
Merged
matbmapspeople merged 11 commits intomainfrom Mar 27, 2026
Merged
Conversation
… and bottom controls overlap handling
…selector is expanded
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects when the Floor Selector opens and visually overlaps desktop bottom-right controls; observes DOM changes, measures bounding rectangles, and conditionally hides/disables the bottom-right controls while the selector is expanded. Changes
Sequence Diagram(s)sequenceDiagram
participant FloorSelector as Floor Selector
participant Hook as useFloorSelectorToggleObserver
participant MapControls as MapControls
participant Browser as Browser (resize/orientation)
FloorSelector->>Hook: DOM mutation (class change)
Hook->>Hook: detect .mi-floor-selector__button--open
Hook-->>MapControls: setIsFloorSelectorExpanded(true) and schedule measureOverlap
MapControls->>MapControls: measureOverlap (read bounding rects of floorSelectorRef & bottomControlsRef)
alt overlap detected
MapControls->>MapControls: setIsFloorSelectorExpanded(true)
MapControls->>MapControls: add class map-controls-container--floor-selector-open
else no overlap
MapControls->>MapControls: setIsFloorSelectorExpanded(false)
MapControls->>MapControls: remove class
end
Browser->>MapControls: resize / orientationchange
MapControls->>MapControls: re-run measureOverlap if selector open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… expanded Floor Selector and Zoom Controls/Full Screen button
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/map-template/src/components/MIMap/MapControls/MapControls.jsx`:
- Around line 218-266: The effect that measures overlap (useEffect using
floorSelectorRef, bottomControlsRef, overlapTimerRef, MutationObserver, and
setIsFloorSelectorExpanded) only listens for class mutations and only reruns for
mapsIndoorsInstance/mapInstance; update it to also recompute on viewport/layout
changes by adding window 'resize' and 'orientationchange' listeners (and
visualViewport 'resize' if available) that call the existing checkOverlap
function, run checkOverlap once on mount to seed state, include isDesktop and
isKiosk in the effect dependency array so layout-mode transitions retrigger the
effect, and ensure all added listeners and overlapTimerRef timeouts are removed
in the cleanup along with observer.disconnect().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acca17cc-99b2-4068-b19f-4d2ffe5c1862
📒 Files selected for processing (2)
packages/map-template/src/components/MIMap/MapControls/MapControls.jsxpackages/map-template/src/components/MIMap/MapControls/MapControls.scss
packages/map-template/src/components/MIMap/MapControls/MapControls.jsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/map-template/CHANGELOG.md`:
- Line 12: Change the typo in the changelog entry by replacing the truncated
word "butto" with the correct word "button" in the line that reads 'An issue
when expanded Floor Selector would overlap Zoom Controls and Full Screen
butto.'; ensure the corrected sentence reads 'An issue when expanded Floor
Selector would overlap Zoom Controls and Full Screen button.' and keep
punctuation and capitalization consistent with the surrounding entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45507146-a744-430f-b792-aa1ad2b658a7
📒 Files selected for processing (1)
packages/map-template/CHANGELOG.md
…responsiveness. Added a measureOverlap function to handle overlap checks and updated the observer for class changes. Adjusted event listeners for viewport changes to ensure accurate overlap measurement when the floor selector is open.
packages/map-template/src/components/MIMap/MapControls/MapControls.jsx
Outdated
Show resolved
Hide resolved
…le observation, improving overlap detection and code organization. The new useFloorSelectorToggleObserver hook encapsulates the logic for monitoring the floor selector's state and measuring overlap with bottom controls.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/map-template/src/hooks/useFloorSelectorToggleObserver.js (2)
28-29: Consider resetting state when button element is not found.If the button element is not found (line 29 early return), the
isFloorSelectorOpenRefretains its previous value andsetIsFloorSelectorExpandedis not called, potentially leaving stale state. This could happen if the floor selector is removed from DOM while open.♻️ Proposed fix
const button = floorSelector.querySelector('.mi-floor-selector__button'); - if (!button) return; + if (!button) { + isFloorSelectorOpenRef.current = false; + setIsFloorSelectorExpanded(false); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/map-template/src/hooks/useFloorSelectorToggleObserver.js` around lines 28 - 29, When the queried button (const button = floorSelector.querySelector('.mi-floor-selector__button')) is missing and the function currently returns early, reset the component state to avoid stale values: set isFloorSelectorOpenRef.current = false and call setIsFloorSelectorExpanded(false) before returning so the ref and UI state are cleared; also ensure any related cleanup (e.g., disconnecting observers) still runs if applicable.
39-39: Magic number for animation delay.The 50ms delay assumes the floor selector expansion animation completes within this time. Consider extracting this to a named constant or documenting the assumption. If the animation duration changes in the future (in
packages/components/src/components/floor-selector/floor-selector.tsx), this value may need adjustment.♻️ Proposed fix
+// Delay to allow floor selector expansion animation to complete before measuring overlap +const ANIMATION_SETTLE_DELAY_MS = 50; + export function useFloorSelectorToggleObserver({ floorSelectorRef, setIsFloorSelectorExpanded, measureOverlap, mapsIndoorsInstance, mapInstance }) { const overlapTimerRef = useRef(null); const isFloorSelectorOpenRef = useRef(false); // ... later in onClassChange: - overlapTimerRef.current = setTimeout(measureOverlap, 50); + overlapTimerRef.current = setTimeout(measureOverlap, ANIMATION_SETTLE_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/map-template/src/hooks/useFloorSelectorToggleObserver.js` at line 39, The hard-coded 50ms delay in useFloorSelectorToggleObserver (overlapTimerRef.current = setTimeout(measureOverlap, 50)) is a magic number tied to the floor selector animation; extract this into a named constant (e.g., FLOOR_SELECTOR_ANIMATION_DELAY or FLOOR_SELECTOR_MEASURE_DELAY) near the top of useFloorSelectorToggleObserver and replace the literal 50 with that constant, and/or read the duration from a shared constant or prop defined in the floor-selector component (floor-selector.tsx) so changes to the component animation duration stay in sync; update any comments to document the assumption and reference measureOverlap and overlapTimerRef so future maintainers know where to adjust the delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/map-template/src/hooks/useFloorSelectorToggleObserver.js`:
- Around line 28-29: When the queried button (const button =
floorSelector.querySelector('.mi-floor-selector__button')) is missing and the
function currently returns early, reset the component state to avoid stale
values: set isFloorSelectorOpenRef.current = false and call
setIsFloorSelectorExpanded(false) before returning so the ref and UI state are
cleared; also ensure any related cleanup (e.g., disconnecting observers) still
runs if applicable.
- Line 39: The hard-coded 50ms delay in useFloorSelectorToggleObserver
(overlapTimerRef.current = setTimeout(measureOverlap, 50)) is a magic number
tied to the floor selector animation; extract this into a named constant (e.g.,
FLOOR_SELECTOR_ANIMATION_DELAY or FLOOR_SELECTOR_MEASURE_DELAY) near the top of
useFloorSelectorToggleObserver and replace the literal 50 with that constant,
and/or read the duration from a shared constant or prop defined in the
floor-selector component (floor-selector.tsx) so changes to the component
animation duration stay in sync; update any comments to document the assumption
and reference measureOverlap and overlapTimerRef so future maintainers know
where to adjust the delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 091cba14-d5be-4f17-80de-0ef1145b0615
📒 Files selected for processing (3)
packages/map-template/CHANGELOG.mdpackages/map-template/src/components/MIMap/MapControls/MapControls.jsxpackages/map-template/src/hooks/useFloorSelectorToggleObserver.js
…server` and its integration in MapControls for improved floor selector functionality.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/map-template/CHANGELOG.md (1)
12-14: Consider restructuring to clarify the fix vs. implementation.While the information requested by the reviewer has been added, the current structure lists implementation details (lines 13-14) as separate bullet points under "Fixed," which typically describes user-facing issues. Consider integrating these details into the first bullet for better clarity:
📝 Suggested restructuring
-- An issue when expanded Floor Selector would overlap Zoom Controls and Full Screen button. -- Added a new custom hook `useFloorSelectorToggleObserver.js`. -- Added newly created observer to MapControls.jsx +- An issue when expanded Floor Selector would overlap Zoom Controls and Full Screen button. Resolved by introducing a custom hook (`useFloorSelectorToggleObserver.js`) and integrating it into `MapControls.jsx` to detect overlap and conditionally hide controls.This preserves all the technical details while making it clear they're part of the implementation of the fix, not separate fixes themselves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/map-template/CHANGELOG.md` around lines 12 - 14, Update the CHANGELOG.md "Fixed" section so the user-facing fix and its implementation details are combined into a single bullet: mention the overlap bug ("expanded Floor Selector would overlap Zoom Controls and Full Screen button") and append the implementation notes that you added the custom hook useFloorSelectorToggleObserver.js and wired it into MapControls.jsx as the mechanism that resolves the issue, rather than listing those as separate bullets; keep the wording concise and clearly indicate they are part of the same fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/map-template/CHANGELOG.md`:
- Around line 12-14: Update the CHANGELOG.md "Fixed" section so the user-facing
fix and its implementation details are combined into a single bullet: mention
the overlap bug ("expanded Floor Selector would overlap Zoom Controls and Full
Screen button") and append the implementation notes that you added the custom
hook useFloorSelectorToggleObserver.js and wired it into MapControls.jsx as the
mechanism that resolves the issue, rather than listing those as separate
bullets; keep the wording concise and clearly indicate they are part of the same
fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef6f1988-a63b-4cec-b6c8-41db8af4ad53
📒 Files selected for processing (1)
packages/map-template/CHANGELOG.md
…or fallback to default value
…ing via App Config, defaulting to 10.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/map-template/src/components/MapTemplate/MapTemplate.jsx`:
- Around line 526-527: The inline comment referencing ignoreViewportBounds is
stale—either remove it or restore its behavior; update the comment near the
minZoom assignment (the minZoom: appConfig?.appSettings?.minZoom ??
ZoomLevelValues.minZoom line) to reflect current behavior (no mention of
ignoreViewportBounds) or reintroduce logic that checks an ignoreViewportBounds
flag and sets minZoom to ZoomLevelValues.minZoom when true; locate the minZoom
assignment in MapTemplate.jsx and either delete/replace the comment or implement
the ignoreViewportBounds check around minZoom to match intended functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 031d67d6-5ca5-40cc-9053-a5d9557e720a
📒 Files selected for processing (1)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx
Lucaci-Andrei
approved these changes
Mar 27, 2026
matbmapspeople
deleted the
feature/matb/fix_floor_selector_expansion_on_small_screens
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit