Skip to content

feat(slider): add Slider atom component#242

Merged
egdev6 merged 8 commits into
mainfrom
feat/slider-atom-integration
May 30, 2026
Merged

feat(slider): add Slider atom component#242
egdev6 merged 8 commits into
mainfrom
feat/slider-atom-integration

Conversation

@ludevdot
Copy link
Copy Markdown
Collaborator

Summary

Add the Slider atom as a complete component PR for issue #29, including Radix-based behavior, range support, Progress-aligned color/rounded variants, tests, and Storybook examples.

Closes #29

Type of change

  • 🧩 New component
  • 🐛 Bug fix
  • 🎨 Design tokens
  • ♿ Accessibility
  • 🏗️ Infrastructure
  • 📚 Documentation

Repository checklist

  • PR title follows Conventional Commit format: <type>(<optional scope>): <description>
  • Commit messages follow the same commitlint-enforced format
  • PR description links the issue with Closes #NNN
  • Security checks pass, or any false positives are documented in the PR

Component checklist (skip if not applicable)

  • Follows the 5-file structure (types.ts, use*.ts, Component.tsx, index.ts, *.stories.tsx)
  • CVA variants defined in types.ts, not inline
  • No hardcoded colors — uses tokens from theme.css
  • No any types — TypeScript strict
  • ARIA attributes present and correct
  • Keyboard navigable (Tab, Enter, Escape where applicable)
  • Dark mode works correctly
  • Storybook stories cover: default, variants, states (hover, focus, disabled)
  • Tests added or updated

How to test

  1. pnpm test -- Slider.test.tsx
  2. pnpm test
  3. pnpm exec tsc --noEmit
  4. pnpm run storybook
  5. Review Atoms/Slider stories: Default, Controlled, Range, Disabled, Sizes, Colors, Rounded, WithExternalLabel, WithFieldDescription.

Screenshots / recordings (if applicable)

Draft PR: screenshots can be added after visual review if needed.

Notes for reviewer

  • This replaces the previous chained/tracker approach after @egdev6 requested one complete Slider PR for combined code + visual review.
  • Slider uses @radix-ui/react-slider for pointer, drag, focus, keyboard, range, and disabled behavior.
  • Supports single-value and explicit two-value range mode with thumbLabels.
  • Includes Progress-aligned color and rounded variants.
  • Judgment Day dual review reached clean approval before opening this draft.

@ludevdot ludevdot added the type:feature Feature changes label May 29, 2026
@ludevdot ludevdot self-assigned this May 29, 2026
@ludevdot ludevdot marked this pull request as ready for review May 29, 2026 23:57
@ludevdot ludevdot requested a review from egdev6 as a code owner May 29, 2026 23:57
@egdev6
Copy link
Copy Markdown
Member

egdev6 commented May 30, 2026

El componente está muy bien encaminado, pero acá hay un gap de accesibilidad en range mode.

En useSlider, cuando isRange es true, se descarta aria-labelledby, entonces los thumbs quedan nombrados como Minimum price / Maximum price, pero pierden el contexto del label externo, por ejemplo Price range en la story. Para screen readers eso separa los thumbs del campo al que pertenecen.

Podés mantener un label de grupo en el root y sumar una estrategia de label por thumb que combine contexto + nombre distintivo, o agregar IDs ocultos por thumb. También agregaría un test de regresión para defaultValue/value con dos thumbs + aria-labelledby.

@egdev6
Copy link
Copy Markdown
Member

egdev6 commented May 30, 2026

Gracias por el update @ludevdot . Revisé de nuevo el último commit (fix(slider): wrap stories in accessible surface) y ese cambio mejora el framing visual de las stories, pero no resuelve el finding anterior.

El problema sigue estando en useSlider: en range mode aria-labelledby todavía se descarta para los thumbs, y tampoco queda asociado a un root/group semántico. Entonces la story puede mostrar Price range, pero ese label sigue sin quedar programáticamente conectado con Minimum price / Maximum price.

Para cerrar este punto hace falta cambiar la estrategia de labelling en range mode, por ejemplo componiendo aria-labelledby por thumb con el label compartido + un label específico por thumb, y sumar un test de regresión para range + aria-labelledby.

@ludevdot
Copy link
Copy Markdown
Collaborator Author

Gracias no había caído en el range + aria-labelledby

@egdev6 egdev6 merged commit 1f124c7 into main May 30, 2026
9 checks passed
@egdev6 egdev6 deleted the feat/slider-atom-integration branch May 30, 2026 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature Feature changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ATOMS] Slider

2 participants