Skip to content

Fix settings scroll target positioning#1829

Open
aukaukauk wants to merge 3 commits intoSlimeVR:mainfrom
aukaukauk:codex/fix-magnetometer-settings-scroll
Open

Fix settings scroll target positioning#1829
aukaukauk wants to merge 3 commits intoSlimeVR:mainfrom
aukaukauk:codex/fix-magnetometer-settings-scroll

Conversation

@aukaukauk
Copy link
Copy Markdown

@aukaukauk aukaukauk commented May 2, 2026

Summary

  • Fix settings-page scrolling to nested targets.
  • Calculate the target position relative to the scroll container instead of relying on offsetTop.

Why

The magnetometer link from a tracker settings page navigates to /settings/trackers with scrollTo: "mechanics-magnetometer". That target is nested inside the settings page, so offsetTop can be relative to a nearer offset parent and scroll to the wrong place.

Testing

  • Ran pnpm --filter slimevr --dir gui lint.
  • Started the server and GUI locally with a fake UDP tracker that supports magnetometer.
  • Opened the fake tracker's settings page, clicked click here to go to the setting, and confirmed it scrolls to the global magnetometer setting.

AI disclosure

AI-assisted for debugging and PR text, tested locally.

Fixes #1764

@github-actions github-actions Bot added the Area: GUI Related to the GUI label May 2, 2026
@aukaukauk aukaukauk marked this pull request as ready for review May 2, 2026 10:50
@loucass003
Copy link
Copy Markdown
Member

now that aw are not on tauri / have safari in the way.
we can prob use https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTo
which is cleaner imo. Need to confirm with testing

@aukaukauk aukaukauk force-pushed the codex/fix-magnetometer-settings-scroll branch from 414898e to 476826e Compare May 2, 2026 21:10
@aukaukauk
Copy link
Copy Markdown
Author

Agreed, scrollTo is cleaner here. I updated it and re-tested the magnetometer link locally.

@loucass003
Copy link
Copy Markdown
Member

woops my bad. i should have linked https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView scrollIntoView

@aukaukauk
Copy link
Copy Markdown
Author

That makes sense. scrollIntoView({ block: 'start' }) changes the visual offset from the existing padding. Would you prefer using scrollIntoView with CSS scroll-margin-top to preserve that spacing? Personally, I think keeping a bit more top spacing helps place the target closer to the visual center.

@loucass003
Copy link
Copy Markdown
Member

yeah i think the css property is perfect for that. if i recall it used to be that a while back. but got changed for compatibility issues with safari. not an issue anymore

@aukaukauk aukaukauk force-pushed the codex/fix-magnetometer-settings-scroll branch from 476826e to 1658595 Compare May 3, 2026 11:05
@aukaukauk aukaukauk force-pushed the codex/fix-magnetometer-settings-scroll branch from 1658595 to a5b066c Compare May 3, 2026 11:07
Comment thread gui/src/index.scss Outdated
scrollbar-width: thin;
}

.settings-page-layout [id] {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use tailwind classes instead. dedicated css classes should only be declared when necessary

@aukaukauk
Copy link
Copy Markdown
Author

Done, updated with scrollIntoView + scroll-margin-top. I kept the existing 45/80px spacing values.

ref={pageRef}
className={classNames('settings-page-layout', className)}
className={classNames(
'[&_[id]]:scroll-mt-[45px] mobile:[&_[id]]:scroll-mt-[80px]',
Copy link
Copy Markdown
Member

@loucass003 loucass003 May 4, 2026

Choose a reason for hiding this comment

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

im pretty sure you dont need the id selector.
and i would try to find the closest standard margin instead of hardcoding px values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: GUI Related to the GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: "click here to go to the setting" link in an individual tracker magnetometer settings goes to wrong settings point.

2 participants