Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RangeSlider: add disableTooltips option #348

Merged
merged 3 commits into from
May 16, 2024

Conversation

ddol
Copy link
Contributor

@ddol ddol commented May 16, 2024

What changed?

Add option to RangeSlider component to disable tooltips.

  <RangeSlider disableTooltips />

Update documentation to provide example:
Screenshot 2024-05-15 at 7 21 32 PM

How was it tested?

Development server was spun up on branch, changes were validated using:

pnpm i
cd packages/svelte-ux; pnpm dev

Unit tests were run but 6 tests failed. These tests also failed on main, so I'm assuming they are unrelated to this change.

Copy link

changeset-bot bot commented May 16, 2024

🦋 Changeset detected

Latest commit: 5143af7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte-ux Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented May 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-ux ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 3:10am

@techniq
Copy link
Owner

techniq commented May 16, 2024

Works for me, although I wonder if we shouldn't default to not showing tooltips and opting in with showTooltips or similar.

@ddol
Copy link
Contributor Author

ddol commented May 16, 2024

I think the tooltips are a good default as they give more clarity as to the values for the range, which is why I opted for a flag to disable. But I'm happy to change to showTooltips if you prefer.

@techniq
Copy link
Owner

techniq commented May 16, 2024

I'm fine with leaving it enabled by default for now. I took a very quick look at a few other popular component libraries and it seems only MUI and Ant have tooltip support, MUI defaults to off (opt in with valueLabelDisplay="auto") and Ant defaults to on (opt out with tooltip={{ open: false }}

I'd like to revisit RangeInput / RangeField / etc (#54) at some point and make it both simplifier and more flexible, but no need to tackle that now.

Thanks for the PR!

@techniq
Copy link
Owner

techniq commented May 16, 2024

Oh, could you add a changeset (pnpm changeset)?

@techniq techniq merged commit d513f4d into techniq:main May 16, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request May 16, 2024
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.

2 participants