Slider: RTL support #791

abdel-17 · 2023-12-03T13:39:18Z

Really excited about this pr! 👀

Added two more orientations for sliders, horizontal-rl and vertical-tb, effectively adding RTL support for sliders with an added bonus of top-to-bottom orientation for vertical sliders. I also documented the two orientations.

I manually tested all 4 orientations with and without ticks. Everything seems to be placed correctly and keyboard shortcuts work. I haven't added any automated tests, though.

changeset-bot · 2023-12-03T13:39:21Z

🦋 Changeset detected

Latest commit: 65b4cde

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

This PR includes changesets to release 1 package

Name	Type
@melt-ui/svelte	Minor

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

huntabyte · 2023-12-03T18:15:35Z

Hey @abdel-17 thanks for taking the time to tackle this!

I think a better approach in terms of DX would be to add a dir prop that accepts either 'ltr' | 'rtl' (defaulting to 'ltr'). And then instead of the switch check for all the orientations, we can have the defaults as they were and add a conditional if (dir === 'rtl') and handle the logic differently there!

Alternatively, we could make the prop rtl: boolean!

The reasoning for this is that the orientation doesn't actually change with RTL, but rather the behavior of that orientation, so it doesn't make much sense to couple them like that!

This would clean things up and enable devs to implement things like a "toggle rtl" much easier since we could add that same prop to all other components as we add RTL support and it will be a matter of changing a single prop!

abdel-17 · 2023-12-03T18:28:00Z

Hey @huntabyte, dir="rtl" makes sense for horizontal sliders, but what about vertical sliders? dir="rtl" doesn't make sense in that context. Should we add dir="ttb"? What if the user passes orientation="horizontal" and dir="ttb"? If you have any suggestions, I'm willing to modify the code.

huntabyte · 2023-12-03T23:41:20Z

I know for a fact we should have either a dir or rtl prop, likely rtl to keep it simple which we can apply to basically every builder as we add RTL support and ensure a consistent DX across the builders.

My thinking behind the whole rtl: boolean idea is that I imagine having some sort of app-wide context tracking if the user is in RTL mode. Then you'd just be able to pass that boolean state into all the builders being used across the app, where everything would just work rather than having to jump through hoops on a per-builder basis if that makes sense.

I noticed on Discord you sent the following:

It seems that, for vertical writing modes, "rtl" does affect orientation. "vertical" with ltr is top-down while with rtl it''s bottom-up. I'm thinking we should support vertical sliders that start from the top, but I'm not sure if it should be another prop or not.

With this being the case, then we only really need rtl. If rtl && orientation === 'vertical' we do top to bottom, if horizontal we do right to left. So in this sense it doesn't really mean direction of the slider but instead the mode the user is in 😃

huntabyte · 2023-12-04T00:00:42Z

src/docs/content/builders/slider.md

+You can set the `orientation` prop to `horizontal-rl` for right-to-left orientation. Here's an
+example:


Suggested change

You can set the `orientation` prop to `horizontal-rl` for right-to-left orientation. Here's an

example:

To enable Right-to-Left (RTL) support, you can set the `rtl` prop to `true.

Here's an example of a horizontal slider with RTL enabled:

huntabyte · 2023-12-04T00:02:43Z

src/docs/content/builders/slider.md

+Top-to-bottom orientation is also supported by setting `orientation` to `vertical-tb`. Here's an
+example:


Suggested change

Top-to-bottom orientation is also supported by setting `orientation` to `vertical-tb`. Here's an

example:

In a Right-to-Left (RTL) context, vertical sliders function from top to bottom instead of bottom to top. Here's an example of a vertical slider with RTL enabled:

huntabyte · 2023-12-04T00:03:51Z

src/docs/previews/slider/horizontal_rl/tailwind/index.svelte

+		elements: { root, range, thumb },
+		states: { value },
+	} = createSlider({
+		orientation: 'horizontal-rl',


Suggested change

orientation: 'horizontal-rl',

rtl: true,

orientation: "horizontal"

huntabyte · 2023-12-04T00:04:18Z

src/docs/previews/slider/vertical_tb/tailwind/index.svelte

+		elements: { root, range, thumb },
+		states: { value },
+	} = createSlider({
+		orientation: 'vertical-tb',


Suggested change

orientation: 'vertical-tb',

rtl: true

orientation: 'vertical',

huntabyte · 2023-12-04T00:05:18Z

src/lib/builders/slider/types.ts

@@ -4,7 +4,7 @@ import type { Writable } from 'svelte/store';
 import type { createSlider } from './create.js';
 export type { SliderComponentEvents } from './events.js';

-export type SliderOrientation = 'horizontal' | 'vertical';
+export type SliderOrientation = 'horizontal' | 'horizontal-rl' | 'vertical' | 'vertical-tb';


Suggested change

export type SliderOrientation = 'horizontal' | 'horizontal-rl' | 'vertical' | 'vertical-tb';

export type SliderOrientation = 'horizontal' | 'vertical';

This reverts commit 1ffe7cc.

TGlide

Thank you so much! I left some comments 🙂

TGlide · 2023-12-14T18:13:05Z

src/lib/builders/slider/create.ts

 				disabled: disabledAttr($disabled),
-				'aria-disabled': ariaDisabledAttr($disabled),
+				'aria-disabled': disabledAttr($disabled),
+				// TODO: Is aria-orientation missing here?


So, about that. I did some research and apparently the aria-disabled and aria-orientation attributes should be on the thumb, not the root. That's a breaking change.

No worries, we can change these. We can keep the data-attributes on the root as well though

src/lib/builders/slider/create.ts

TGlide · 2023-12-14T18:19:03Z

src/lib/builders/slider/create.ts

-					style.translate = horizontal ? '-100% 0' : '0 100%';
-				} else if (index !== 0) {
-					style.translate = horizontal ? '-50% 0' : '0 50%';
+				if ($orientation === 'horizontal' && $dir === 'rtl') {


These ifs can be a bit complex too, something similar to what i've shown before would be useful 😄

These ifs can be a bit complex too, something similar to what i've shown before would be useful 😄

I'm not the biggest fan of ternaries, personally, but I'll fix the ifs. I wish JS had native pattern matching or switch statements with multiple values.

Hey! So I just pushed the changes. I chose a different approach from ternary statements. If you still prefer ternaries, I have no problem refactoring the code, but I think the new changes are much more readable than both the ternaries and the if statements.

I do prefer the ternaries, but the direction derived with the switch case is pretty nice too 😄 You can keep it!

TGlide · 2023-12-17T13:53:18Z

src/lib/builders/slider/create.ts

+				// TODO: Consider removing `aria-disabled` from here.
+				// `aria-disabled` doesn't make sense on the root
+				// because the slider `role` is on the thumb.


Let's do it, we can keep the data attribute

Co-authored-by: Thomas G. Lopes <26071571+TGlide@users.noreply.github.com>

abdel-17 added 5 commits December 3, 2023 14:50

Add 'horizontal-rl' and 'vertical-tb' orientations

d04f47a

Fix tick position in 'vertical-tb' orientation

11ffe72

Add documentation for rtl

ec3ec3e

Fix thumb focus ring color

773fe40

Run prettier

b129652

vercel bot temporarily deployed to Preview December 3, 2023 13:43 Inactive

huntabyte requested changes Dec 4, 2023

View reviewed changes

abdel-17 added 2 commits December 4, 2023 12:29

refactor: use 'dir' prop to indicate RTL

7451f99

Run prettier

6e4a692

vercel bot temporarily deployed to Preview December 4, 2023 10:37 Inactive

abdel-17 requested a review from huntabyte December 4, 2023 17:44

fix: add missing "aria-orientation" to slider root

1ffe7cc

vercel bot temporarily deployed to Preview December 4, 2023 18:54 Inactive

Revert "fix: add missing "aria-orientation" to slider root"

4bc7402

This reverts commit 1ffe7cc.

vercel bot temporarily deployed to Preview December 4, 2023 21:53 Inactive

TGlide requested changes Dec 14, 2023

View reviewed changes

refactor: replace complex if statements with switch

d3fa89e

vercel bot temporarily deployed to Preview December 16, 2023 22:55 Inactive

fix: add missing aria attributes

7f1eebd

vercel bot temporarily deployed to Preview December 17, 2023 02:42 Inactive

fix: aria-orientation prop

ed07fa1

vercel bot temporarily deployed to Preview December 17, 2023 02:48 Inactive

TGlide reviewed Dec 17, 2023

View reviewed changes

Create odd-horses-reflect.md

65b4cde

vercel bot temporarily deployed to Preview December 17, 2023 14:06 Inactive

TGlide merged commit 879ac5f into melt-ui:develop Dec 17, 2023
4 checks passed

github-actions bot mentioned this pull request Dec 17, 2023

Version Packages #824

Merged

lolcabanon pushed a commit to lolcabanon/melt-ui that referenced this pull request Dec 28, 2023

Slider: RTL support (melt-ui#791)

0302609

Co-authored-by: Thomas G. Lopes <26071571+TGlide@users.noreply.github.com>

abdel-17 deleted the feat/slider-rtl-support branch February 14, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slider: RTL support #791

Slider: RTL support #791

abdel-17 commented Dec 3, 2023

changeset-bot bot commented Dec 3, 2023 •

edited

Loading

huntabyte commented Dec 3, 2023 •

edited

Loading

abdel-17 commented Dec 3, 2023 •

edited

Loading

huntabyte commented Dec 3, 2023 •

edited

Loading

huntabyte Dec 4, 2023

huntabyte Dec 4, 2023

huntabyte Dec 4, 2023

huntabyte Dec 4, 2023

huntabyte Dec 4, 2023

TGlide left a comment

TGlide Dec 14, 2023

abdel-17 Dec 15, 2023

TGlide Dec 17, 2023

TGlide Dec 14, 2023

abdel-17 Dec 16, 2023 •

edited

Loading

abdel-17 Dec 16, 2023

TGlide Dec 17, 2023

TGlide Dec 17, 2023

		You can set the `orientation` prop to `horizontal-rl` for right-to-left orientation. Here's an
		example:

		Top-to-bottom orientation is also supported by setting `orientation` to `vertical-tb`. Here's an
		example:

	Top-to-bottom orientation is also supported by setting `orientation` to `vertical-tb`. Here's an
	example:
	In a Right-to-Left (RTL) context, vertical sliders function from top to bottom instead of bottom to top. Here's an example of a vertical slider with RTL enabled:

	orientation: 'horizontal-rl',
	rtl: true,
	orientation: "horizontal"

	orientation: 'vertical-tb',
	rtl: true
	orientation: 'vertical',

	export type SliderOrientation = 'horizontal' \| 'horizontal-rl' \| 'vertical' \| 'vertical-tb';
	export type SliderOrientation = 'horizontal' \| 'vertical';

Slider: RTL support #791

Slider: RTL support #791

Conversation

abdel-17 commented Dec 3, 2023

changeset-bot bot commented Dec 3, 2023 • edited Loading

🦋 Changeset detected

huntabyte commented Dec 3, 2023 • edited Loading

abdel-17 commented Dec 3, 2023 • edited Loading

huntabyte commented Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TGlide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdel-17 Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 3, 2023 •

edited

Loading

huntabyte commented Dec 3, 2023 •

edited

Loading

abdel-17 commented Dec 3, 2023 •

edited

Loading

huntabyte commented Dec 3, 2023 •

edited

Loading

abdel-17 Dec 16, 2023 •

edited

Loading