Skip to content

Keep the slider pill inside its track - #97

Open
OSSDiablo wants to merge 2 commits into
SiriusSoftwareLtd:devfrom
OSSDiablo:93-slider-handle-bounds
Open

Keep the slider pill inside its track#97
OSSDiablo wants to merge 2 commits into
SiriusSoftwareLtd:devfrom
OSSDiablo:93-slider-handle-bounds

Conversation

@OSSDiablo

@OSSDiablo OSSDiablo commented Aug 2, 2026

Copy link
Copy Markdown

Summary

The slider's pill draws outside its own track at both ends of the range. On a full-width slider there is enough margin either side to hide it. On a slider inside a group there is not, so the pill draws over the card beside it.

Related issue

Closes #96

Change type

  • Bug fix
  • Feature
  • Refactor or maintenance
  • Build, tooling, or CI
  • Breaking change

Changes

  • src/components/slider.luau: the pill was pinned to the right edge of the fill by a fixed 20px, and the fill was a plain fraction of the track. Both are inset by the pill now, and by half of it for the fill: fillSize returns UDim2.new(ratio, (1 - ratio) * width / 2, 1, 0) and handleOffset returns UDim2.new(1, (1 - ratio) * width / 2, 0.5, 0).
  • At the top of the range the fill is the whole track, so the bar still reads as full with the pill sitting on the end of it. At the bottom the fill reaches the middle of the pill, so the pill has fill behind its left half and none of it shows beside the pill. Halfway along, both land within a pixel of where they always did.
  • _setHeld grows the pill from 35 to 41, which both are measured off, so that path moves them too. Without it, grabbing a pill at the bottom of the range pushes it back outside the track.

Compatibility and breaking changes

  • Breaking change: no
  • Migration required: no
  • Compatibility notes: geometry only. No API, no persisted data, no theme keys. A slider at mid-range is unchanged to the eye.

Validation

  • I ran make ci, including the coverage threshold, or explained below why it does not apply.
  • I added or updated automated or Roblox-hosted tests where practical.
  • I completed applicable manual testing in Roblox Studio or the affected environment.
  • I tested the affected compatibility paths.

Test scenarios and results

  • make ci: 230 tests pass, coverage 82.68% against the 82.68% baseline in this branch.
  • Three new specs: the fill and the pill at both ends and at the middle, the held size change moving both, and a minimal slider in a group held to the same bounds.
  • Exercised in a live client: two, three and four column groups with every slider parked at an end, minimal sliders in a row, full-width sliders for comparison, and dragging one to both ends and back while holding it.

Documentation coordination

  • Public documentation impact: none. Nothing public changed, only where two frames sit.
  • Companion docs issue or pull request: not applicable.
  • In-repository comments, examples, or types updated: comments only, above the two helpers.

UI evidence

Before After
https://github.com/user-attachments/assets/e8d7998c-4061-4363-8a39-8b0a09c5985a https://github.com/user-attachments/assets/c0b15097-0605-4e81-a74d-5d4ecca08f08

Before: pills breaking out of their cards at both ends. Music at 0 draws over the card to its left, Prediction and Field Of View Radius at the top of their ranges draw past the right edge, and Jump at the bottom hangs off the left.

After: every pill inside its own card. The bar is full at the top with the pill on the end of it, and at the bottom the fill reaches the middle of the pill with nothing showing beside it. Dragging one to both ends while holding it is at the end of the clip.

Checklist

  • This pull request targets dev.
  • The change is focused and contains no unrelated work.
  • I reviewed my own changes.
  • Tests cover the change where practical.
  • Coverage did not drop below the enforced threshold.
  • Public documentation is unaffected, or I linked companion work in SiriusSoftwareLtd/docs.
  • Required code comments, examples, and types match the resulting behavior.
  • I did not commit standalone documentation changes that belong in the docs repository.
  • I did not commit generated files such as roblox.yml, sourcemap.json, globalTypes.d.luau, or build/.
  • This pull request contains no publicly disclosed vulnerability details.

The pill was pinned to the right edge of the fill by a fixed 20px, and
the fill was a plain fraction of the track. At the top of the range that
put the pill's right edge 20px past the end of the track, and at the
bottom, where the fill has no width at all, it started 15px before the
track began. A full-width slider has margin either side to hide that. A
slider in a group does not, so the pill drew over the card beside it.

Both are inset by the pill now, and by half of it where the fill is
concerned. At the top the fill is the whole track, so the bar still reads
as full with the pill on the end of it. At the bottom the fill reaches
the middle of the pill, so the pill has green behind its left half and
nothing shows beside it. Halfway along, both land within a pixel of where
they always did.

Holding grows the pill from 35 to 41, which both are measured off, so
that path moves them too.

@MaxTCodes MaxTCodes left a comment

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.

I found two regressions in the new slider geometry that should be fixed before merging.

  • The handle now follows an inset path, but pointer input still maps across the full track. This can change the slider value when a user starts dragging the handle near either end.

  • The new handle-position tween also conflicts with the multi-property held-state tween. A drag update can cancel that tween before the held size and transparency finish.

The bounds fix itself looks sound, and the added tests cover the static geometry. They do not cover these two runtime interaction cases.

Requesting changes for the two inline issues mentioned above.

Comment thread src/components/slider.luau
Comment thread src/components/slider.luau
The pill travels an inset path, half a pill in from each end, but the
pointer was still mapped across the whole track. Grabbing the pill at
either end moved the value before the drag did anything.

- map the pointer over the same inset span, clamping the end caps to
  the ends of the range
- fall back to the whole track when the track is narrower than the pill
- split the held tween from the follow: the hold owns Size and the
  transparencies, _renderProgress owns Position and the fill, so a drag
  update no longer cancels the grow partway
@OSSDiablo

Copy link
Copy Markdown
Author

Both fixed.

  • the pointer maps over the same inset span the pill travels now, clamped so the end caps still land on the minimum and maximum. Falls back to the whole track if the track is ever narrower than the pill, since the span would go negative and flip the slider
  • split the two tweens by property. The hold owns Size and the transparencies, _renderProgress owns Position and the fill, and _setHeld hands the move over instead of writing Position itself. Nothing overlaps, so a drag update can't cancel the grow

Specs for both, plus one for the narrow track. The existing drag spec changed with it: at x=150 on a 200px track it's 19 rather than 25, which is (150 - 100 - 41/2) / (200 - 41) with the held pill.

@MaxTCodes MaxTCodes left a comment

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.

Re-checked the latest changes.

The two issues from my previous review are fixed, and the added regression tests cover both cases. The pointer now follows the same inset path as the slider handle, and the held-state tween no longer conflicts with drag updates.

The required Studio validation is documented in the PR, and CI is passing.

I do not see any remaining code issues blocking this PR. Approved on my end.

We will still need @jensonhirst to sign off on the PR. He will be responsible for merging it once he is happy with the changes. LGTM 👍🏽

@jensonhirst

Copy link
Copy Markdown
Member

ratioFromPointer guards the case where the pill is wider than the track. fillSize and handleOffset read the same two numbers and don't guard it. Worth making that consistent across all three, or handling the narrow case once at the caller so none of them need to. Doesn't have to be this PR.

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.

3 participants