feat: Slider component #164

raechelo · 2025-01-13T17:55:12Z

Closes

✅ Pull Request Checklist:

Included link to corresponding GitHub Issue.
The commit message follows conventional commit extended guidelines.
Added/updated unit tests and storybook for this change (for bug fixes / features).
Added/updated documentation (for bug fixes / features)
Filled out test instructions.

📝 Test Instructions:

open the ladle page and view the Slider component. There are a handful of stories for review, including:

controlled range slider
controlled single value slider
uncontrolled range slier
uncontrolled single value slider
slider with output (RAC components indicate an output value, but our designs indicate an input value. The above components feature the input, but that inclusion can be toggled in the props menu by toggling includeTextField and includeOutputField

❓ Does this PR introduce a breaking change?

Yes
No

💬 Other information

I'm happy to hear any and all feedback, especially on variable names and naming conventions. I'm sure there are some internal practices I was not aware of!

I didn't have a ticket for this, because I don't have Jira access yet, but here is the screenshot I was given and attempted to match.

raechelo · 2025-01-13T18:04:38Z

packages/design-system/src/components/slider/slider.stories.tsx

+                <div
+                  className={sliderBar}
+                  style={{
+                    position: 'absolute',
+                    left: state.getThumbValue(1)
+                      ? `${state.getThumbValue(0)}%`
+                      : 0,
+                    width: state.getThumbValue(1)
+                      ? `${(state.getThumbValue(1) || 0) - (state.getThumbValue(0) || 0)}%`
+                      : `${state.getThumbValue(0)}%`,
+                  }}
+                />


i tried a handful of ways to implement the bar, and I doubt this is the correct way. Any advice is greatly appreciated! I used this example for this implementation.

Because of how this is implemented, it does mean there are two slider bars when using the Range Slider. 😬😞

Yea, let's go over this when we have time

raechelo · 2025-01-13T18:06:07Z

packages/design-system/src/components/slider/slider.tsx

@@ -0,0 +1,312 @@
+import {


i have a feeling i didn't organize this file properly, please lmk if there is a better way to do this!

raechelo · 2025-01-13T18:09:23Z

packages/design-system/.ladle/theme/slider.css.ts

@@ -0,0 +1,145 @@
+import { style } from '@vanilla-extract/css';


I eyeballed all the styles from the pic, so I doubt they're fully accurate. If anything should be tweaked, I am happy to do so!

raechelo · 2025-01-13T18:21:15Z

packages/design-system/src/components/slider/slider.stories.tsx

+            {({ state }: SliderRenderProps) => (
+              <>
+                <div
+                  className={sliderBar}


i think i applied this incorrectly, but was unable to get it working with the same method the Picker stories were using. User error, i'm sure 😆

… into ds-slider

raechelo · 2025-01-13T21:48:37Z

packages/design-system/src/components/slider/slider.css.ts

+  },
+  thumb: {
+    container: style({
+      ' input': {


looks like this is the line that is causing the build to fail. I wasn't sure how to properly address this issue, because the RAC component has an grandchild element of input, which holds a 90px width. The tooltip in the story is hovering over the input rather than the main level component, thus the width change. Below are a few pictures to better explain.

fwiw, this is how RAC implements the component. You can see the same input width in this screenshot:

We'll look at this together. But the rule that you appear to be breaking is that (aside from pseudo selectors), styles can only target the current element:
https://vanilla-extract.style/documentation/styling/#complex-selectors

raechelo · 2025-01-13T22:30:06Z

packages/design-system/src/components/slider/types.ts

+  };
+  track: {
+    container: string;
+    track: string; // TODO: adjust the styles to use this class


i have the TODO here, but i'm not quite sure i actually need to make that change 🤔💭

packages/design-system/.ladle/theme/slider.css.ts

ArrayKnight · 2025-01-13T22:33:08Z

packages/design-system/.ladle/theme/slider.css.ts

+  sliderTrackStateVars,
+  type ThemeContext,
+} from '../../src';
+import type { SliderThumbState } from '../../src/components/slider/types';


Is there a reason you're not importing from root? I see it commented out above

packages/design-system/.ladle/theme/slider.css.ts

ArrayKnight · 2025-01-13T22:36:42Z

packages/design-system/src/components/index.ts

+  sliderStateVars,
+  sliderThumbStateVars,
+  sliderTrackStateVars,
+  type LabelAlignment,


This should be prefixed with Slider to not cause a naming conflict

ArrayKnight · 2025-01-13T22:38:29Z

packages/design-system/src/components/slider/slider.css.ts

+} from '@vanilla-extract/css';
+import type { SliderClassNames } from './types';
+import { layers, typographyVars } from '../../styles';
+import { containerQueries } from '@/utils';


Let's hold off on using import aliases for the moment

ArrayKnight · 2025-01-14T17:53:13Z

packages/design-system/src/components/slider/types.ts

+
+export type LabelAlignment = 'top' | 'left';
+
+export type SliderRenderProps = RACSliderRenderProps;


You'll likely want to wrap these with AsType

ArrayKnight · 2025-01-14T17:55:15Z

packages/design-system/src/components/slider/types.ts

+  BaseSliderProps;
+
+type BaseSliderThumbProps = {
+  children?: RenderPropsChildren<SliderRenderProps>;


I don't think you need to redefine children here unless you're actually changing the type (which I don't think you are). And then you could simplify why getting rid of the BaseSlideThumbProps and just add the classNames prop directly to the join

This applies below too

ArrayKnight · 2025-01-14T17:58:32Z

packages/design-system/src/hooks/use-defaults/types.ts

@@ -122,6 +129,18 @@ export type DefaultsContext = DefaultsOf<{
  // biome-ignore lint/style/useNamingConvention: component name should be PascalCase
  Select: SelectProps<object>;
  // biome-ignore lint/style/useNamingConvention: component name should be PascalCase
+  Slider: SliderProps;
+  // biome-ignore lint/style/useNamingConvention: component name should be PascalCase


Dear lord, who did this Biome commenting? Can you create an issue to convert to range suppression biomejs/biome#4569 please?

ArrayKnight · 2025-01-14T17:58:44Z

packages/design-system/src/hooks/use-theme/types.ts

@@ -86,6 +87,8 @@ export type ThemeContext = {
  // biome-ignore lint/style/useNamingConvention: component name should be PascalCase
  Select?: SelectClassNames;
  // biome-ignore lint/style/useNamingConvention: component name should be PascalCase
+  Slider?: SliderClassNames;


ArrayKnight · 2025-01-14T17:59:42Z

packages/design-system/src/components/slider/slider.stories.tsx

+                <div
+                  className={sliderBar}
+                  style={{
+                    position: 'absolute',
+                    left: state.getThumbValue(1)
+                      ? `${state.getThumbValue(0)}%`
+                      : 0,
+                    width: state.getThumbValue(1)
+                      ? `${(state.getThumbValue(1) || 0) - (state.getThumbValue(0) || 0)}%`
+                      : `${state.getThumbValue(0)}%`,
+                  }}
+                />


Yea, let's go over this when we have time

Raechel Odom added 9 commits December 23, 2024 16:43

slider component started with types. not fully displayed.

abd40dc

adds range slider story

8b70d81

applies track colors for range + single value sliders

f25bf0e

applies align label prop with css

886dbbf

updates applies styles to separate optional component

b67562b

tooltip working, test added

a18dae6

changes based on feedback

6e47334

addresses types, fixes hover effect

06c562f

removes extra components from test file

a5cdbe9

raechelo commented Jan 13, 2025

View reviewed changes

updates comment to remove mention of deceased sliderbar

2c07332

raechelo commented Jan 13, 2025

View reviewed changes

Merge branch 'main' of https://github.com/gohypergiant/standard-toolkit…

a412473

… into ds-slider

raechelo commented Jan 13, 2025

View reviewed changes

ArrayKnight requested changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Slider component #164

feat: Slider component #164

raechelo commented Jan 13, 2025 •

edited

Loading

raechelo Jan 13, 2025

ArrayKnight Jan 14, 2025

raechelo Jan 13, 2025

raechelo Jan 13, 2025

raechelo Jan 13, 2025

raechelo Jan 13, 2025

raechelo Jan 13, 2025

ArrayKnight Jan 13, 2025

raechelo Jan 13, 2025

ArrayKnight Jan 13, 2025

ArrayKnight Jan 13, 2025

ArrayKnight Jan 13, 2025

ArrayKnight Jan 14, 2025

ArrayKnight Jan 14, 2025

ArrayKnight Jan 14, 2025

ArrayKnight Jan 14, 2025

ArrayKnight Jan 14, 2025

ArrayKnight Jan 14, 2025

		@@ -0,0 +1,145 @@
		import { style } from '@vanilla-extract/css';


		export type LabelAlignment = 'top' \| 'left';

		export type SliderRenderProps = RACSliderRenderProps;

feat: Slider component #164

Are you sure you want to change the base?

feat: Slider component #164

Conversation

raechelo commented Jan 13, 2025 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

❓ Does this PR introduce a breaking change?

💬 Other information

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raechelo commented Jan 13, 2025 •

edited

Loading