fix: correct fullscreen button positioning in map pages#1507
fix: correct fullscreen button positioning in map pages#1507eran132 wants to merge 2 commits intohasadna:mainfrom
Conversation
Changed the expand button from position:fixed to position:absolute so it stays anchored to its map container instead of the viewport. This fixes the button appearing in unexpected positions when scrolling or resizing. Simplified the useConstrainedFloatingButton hook since absolute positioning within the relatively-positioned .map-info container handles constraint automatically. Closes hasadna#1360 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the fullscreen/expand button positioning on map pages by anchoring the button to the map container rather than the viewport, and simplifies the related positioning hook now that viewport-relative constraints are no longer needed.
Changes:
- Changed
.expand-buttonfromposition: fixedtoposition: absoluteso it stays within.map-info. - Simplified
useConstrainedFloatingButtonto only adjust inline offsets for the expanded state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/resources/map.scss |
Anchors the expand button to the map container via absolute positioning within .map-info. |
src/hooks/useConstrainedFloatingButton.ts |
Removes viewport/intersection logic and keeps only expanded-state offset adjustments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isExpanded) { | ||
| buttonElement.style.left = '5px' | ||
| buttonElement.style.bottom = '20px' | ||
| buttonElement.style.top = '' | ||
| } else { | ||
| buttonElement.style.left = '' | ||
| buttonElement.style.bottom = '' | ||
| buttonElement.style.top = '' | ||
| } |
| export function useConstrainedFloatingButton( | ||
| mapContainerRef: RefObject<HTMLDivElement | null>, | ||
| buttonRef: RefObject<HTMLButtonElement | null>, | ||
| isExpanded: boolean, | ||
| ) { |
|
Hi Eren, thanks for all your work. 
|
The scroll-based button movement is intentional behavior to keep the expand button visible without scrolling. Reverted both the CSS and hook changes as the original position:fixed approach is correct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the feedback @AvivAbachi! You're right — the scroll-based movement is intentional. I've reverted both the CSS and hook changes. The original |
NoamGaash
left a comment
NoamGaash
left a comment
There was a problem hiding this comment.
I'm not sure why but this PR seems empty of changes -
Also - you have mentioned a "Test plan" in your PR description and it seems like you didn't finish testing - please complete your PR and ask for a review once you feel it's ready. Also, since you're working from a fork instead of pushing your branch directly to this repo (as we recommend in the contribution guidelines) there's no preview and visual tests can't be executed (forks has no access to the S3 bucket and the Eyes API key). Please provide a screenshot or a video showing the change. Have you verified it looks good on dark/light/rtl/ltr modes?
Thanks!
|
Closing — after reverting per Aviv's feedback, this PR has no changes. The original button positioning is correct as-is. |
Summary
position: fixedtoposition: absoluteso it stays anchored to its map containeruseConstrainedFloatingButtonhook — the complex viewport-relative calculations are no longer needed since absolute positioning within.map-info(which hasposition: relative) handles containment automaticallyCloses #1360
Test plan
🤖 Generated with Claude Code