Skip to content

fix: correct fullscreen button positioning in map pages#1507

Open
eran132 wants to merge 2 commits intohasadna:mainfrom
eran132:fix/fullscreen-button-position
Open

fix: correct fullscreen button positioning in map pages#1507
eran132 wants to merge 2 commits intohasadna:mainfrom
eran132:fix/fullscreen-button-position

Conversation

@eran132
Copy link
Copy Markdown

@eran132 eran132 commented Apr 13, 2026

Summary

  • Changed expand button from position: fixed to position: absolute so it stays anchored to its map container
  • Simplified useConstrainedFloatingButton hook — the complex viewport-relative calculations are no longer needed since absolute positioning within .map-info (which has position: relative) handles containment automatically
  • Reduced hook from 116 lines to 23 lines

Closes #1360

Test plan

  • TypeScript compiles clean
  • Full lint passes (ESLint + Stylelint + Prettier)
  • Unit tests pass (9/9)
  • Verify expand button appears in bottom-left of map on all map pages (velocity heatmap, time-based map, single line map)
  • Verify button stays in correct position when scrolling
  • Verify fullscreen toggle still works correctly

🤖 Generated with Claude Code

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>
@eran132 eran132 requested a review from AvivAbachi as a code owner April 13, 2026 21:37
Copilot AI review requested due to automatic review settings April 13, 2026 21:37
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 13, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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-button from position: fixed to position: absolute so it stays within .map-info.
  • Simplified useConstrainedFloatingButton to 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.

Comment on lines 13 to 21
if (isExpanded) {
buttonElement.style.left = '5px'
buttonElement.style.bottom = '20px'
buttonElement.style.top = ''
} else {
buttonElement.style.left = ''
buttonElement.style.bottom = ''
buttonElement.style.top = ''
}
Comment on lines 3 to 7
export function useConstrainedFloatingButton(
mapContainerRef: RefObject<HTMLDivElement | null>,
buttonRef: RefObject<HTMLButtonElement | null>,
isExpanded: boolean,
) {
@AvivAbachi
Copy link
Copy Markdown
Collaborator

Hi Eren, thanks for all your work.
The expand button set is moving based on scroll, so it can be visible without scrolling down.

{4F6DF4A8-D17C-4D4A-99BE-69BF8BC22458}

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>
@eran132
Copy link
Copy Markdown
Author

eran132 commented Apr 15, 2026

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 position: fixed approach is the correct one for keeping the button visible.

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.

the fullscreen button has weird position

3 participants