feat: add a new switch component #960
Conversation
🦋 Changeset detectedLatest commit: efbbbc9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
commit: |
…/qwik-ui into add_component_switch
|
Hey Jerry! Just went through the implementation. We're getting close! A couple of areas of improvement:
https://kobalte.dev/docs/core/components/switch/ I think this is a good source of inspiration to look at. Along with a set of resources I created here: For example, I think it makes a lot of sense to have a new component tied to a piece of markup for the thumb, rather than having it be on a pseudo element, since that would follow the principles of composability. The idea that pieces of markup and components can blend in with each other in a straightforward way. Atomic Design is a good read if you'd like to look further into it. I think TDD / adding some tests would help a lot to prevent regressions and keep things robust. We need some tests before we can put the switch component in the beta stage. |
|
Thank you for reviewing, I will fix it soon |
|
…/qwik-ui into add_component_switch
I done |
|
it's weird, I have no idea with this error. I didn’t delete the tailwind file |
| } | ||
| }); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
This breaks the rule of composability
See: https://qwik.design/contributing/composition/
I would suggest these becoming different component pieces
| ({ checked, disabled, onChange$, ...rest }: SwitchProps) => { | ||
| useStyles$(styles); | ||
| const defaultChecked = checked || rest['bind:checked']?.value; | ||
| const checkedState = useSignal(defaultChecked || false); |
There was a problem hiding this comment.
We have a hook now that gives you one source of truth! useBoundSignal
https://qwik.design/contributing/state/#binds-in-qwik
the first param is the given signal of the consumer (passed to bind:checked)
| onClick$={[handleClickSync$, handleClick$]} | ||
| > | ||
| <input | ||
| {...rest} |
There was a problem hiding this comment.
Looks like we need 4 pieces here:
Switch.Track, Switch.Thumb, Switch.Trigger (a button or div), and Switch.HiddenInput
But this is based on the current structure. I would follow the research process here:
| THEN the onChange callback should be triggered`, async ({ page }) => { | ||
| const { driver: d } = await setup(page, 'hero'); | ||
| await expect(d.getTriggerlaBle()).toHaveText('test0'); | ||
| await d.getTrigger().click({force: true}); |
There was a problem hiding this comment.
shouldn't need force true here
| const { driver: d } = await setup(page, 'hero'); | ||
| await expect(d.getTriggerlaBle()).toHaveText('test0'); | ||
| await d.getTrigger().click({force: true}); | ||
| await expect(d.getTriggerlaBle()).toHaveText('test1'); |
There was a problem hiding this comment.
we shouldn't be testing an onchange callback on a hero example
| await expect(d.getTrigger()).not.toBeChecked(); | ||
| }); | ||
|
|
||
| test(` |
There was a problem hiding this comment.
This test can probably be removed, it's not really doing much
| }); | ||
|
|
||
| test(` | ||
| GIVEN a defaultChecked switch |
There was a problem hiding this comment.
GIVEN a switch that is initially checked
| await expect(d.getTriggerlaBle()).not.toBeNull(); | ||
| }); | ||
|
|
||
| test(` |
There was a problem hiding this comment.
Good point on this test, perhaps each of the components should have one test verifying all the main pieces have an attribute on them so that we know props is spread
|
I think we could merge this in as a draft state if you wanted to start consuming the current version. With the current feedback above a lot of changes are still needed to be prod ready. (and processes have improved since this PR) Props to the continued effort on this @JerryWu1234 💪 . Many parts of this were not documented, and have since been documented in qwik.design I will start writing some docs on form support as well |
|
Hey Jerry! Here's the new docs on form handling: |
…/qwik-ui into add_component_switch
What is it?
Switch Component Implementation
This PR implements a headless Switch component with the following features:
Features
role="switch"bind:checkedsignaldisabledcheckedonChange$onClick$autoFocusdefaultCheckedTest Coverage
Component Structure
SwitchRoot: Main component wrapperSwitchInput: Core switch functionalitySwitchLabel: Accessible labelingUsage Example
This implementation follows WAI-ARIA best practices and provides a flexible foundation for custom styling while maintaining accessibility.
Why is it needed?
Checklist:
pnpm changeand documented my changes