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

Switch proposal #785

Merged
merged 38 commits into from Feb 29, 2024
Merged

Switch proposal #785

merged 38 commits into from Feb 29, 2024

Conversation

gfellerph
Copy link
Contributor

@gfellerph gfellerph commented Jul 29, 2023

This is a first draft for a proposal for standardising the <switch> component, submitted by recommendation of @gregwhitworth.

There is a parallel effort on standardising the switch component at whatwg/html#9546.


Limitations:

- The content on both sides cannot be wider than the tumb. Otherwise, the thumb is no longer capable of hiding the content of the inactive track content.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest the "on" and "off" text might be better to either not include with this control - or rethink where this text actually belongs if rendered by the browser. Otherwise, having a requirement such as this will make localized strings difficult to handle - along with making it unlikely to be able to support WCAG's text spacing rule

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your inputs! This is intended to be an author defined text, either through a slot or other means. I'll clarify that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottaohara IMO, this would go like so:

<switch>On</switch>

That said @gfellerph this highlights that you may actually want two parts for on/off. The browser will then handle styling when they are toggled. So something like this:

<switch>
      # --- UA Shadow <label>
            <toggled>On</toggledlabe>
            <untoggled>Off</untoggled>
      </label>
</switch>

I hit this same issue when sketching out <checkbox> as you want not only localization but also people will want to put SVGs and other content within the different states declaratively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to the anatomy section, thanks for the input.

Copy link
Collaborator

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice explainer! I think this LGTM to land now, and we can iterate on the concepts and discuss them in issues. @scottaohara's comments are good, so it'd be good to incorporate answers/updates for those.

site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
@josepharhar
Copy link
Collaborator

Thanks for writing this up!
I'd like to see more use cases and examples to justify the track, trackslide, and thumb elements.
If it turns out that we can do everything we want without the need for those elements, then maybe it would be possible to build it on the newly proposed switch which extends the input element: whatwg/html#9546
Otherwise, we would need to create a separate element.

I agree with mason that we can merge this now regardless

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image for the design is not rendering in the deployment, unsure what the issue is.

site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
Limitations:

- The thumb needs to be at least the height of the track in order to hide the boundary between active and inactive track side underneath.
- It's unclear how to style the track and slider if the track content can be of arbitrary length.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open an issue for the default user-agent styles of the form control as a quick idea to a solution of this would be a minmax(min-content, <whatever the user-agent defines>) for example.

Copy link
Contributor Author

@gfellerph gfellerph Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

This highlights the issue a bit better. In order for the animation to work, the track sides need to have the same width. One side is dependent on the others width and I was not sure how this would work.

But probably it is already too detailed for this stage, therefore I think I'm going to remove the passage.

A similar issue appears with the other type of animation, where the thumb is supposed to hide inactive track content underneath.

image


Limitations:

- The content on both sides cannot be wider than the tumb. Otherwise, the thumb is no longer capable of hiding the content of the inactive track content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottaohara IMO, this would go like so:

<switch>On</switch>

That said @gfellerph this highlights that you may actually want two parts for on/off. The browser will then handle styling when they are toggled. So something like this:

<switch>
      # --- UA Shadow <label>
            <toggled>On</toggledlabe>
            <untoggled>Off</untoggled>
      </label>
</switch>

I hit this same issue when sketching out <checkbox> as you want not only localization but also people will want to put SVGs and other content within the different states declaratively.

site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some suggested changes / missing bits and questions about some stuff i'm wondering about / have concerns without better understanding

site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
site/src/pages/components/switch.explainer.mdx Outdated Show resolved Hide resolved
gfellerph and others added 2 commits November 17, 2023 09:07
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
gfellerph and others added 5 commits November 17, 2023 16:06
Co-authored-by: Keith Cirkel <keithamus@users.noreply.github.com>
As discussed on the initial pull request (openui#785 (comment)), treating the switch as a button with submission capabilities has potential accessibility issues as mentioned by @scottaohara.
@lukewarlow
Copy link
Collaborator

Is this ready to land as an initial explainer and then add to it in follow ups?

@gfellerph
Copy link
Contributor Author

gfellerph commented Nov 30, 2023

Is this ready to land as an initial explainer and then add to it in follow ups?

I'd like to implement or resolve the open comments from @scottaohara and @keithamus first (thank you for taking the time to have a look at this). Also, the resolution of #959 will have influence on some sections.

And I'm not allowed to merge PRs here ^^

@lukewarlow
Copy link
Collaborator

That makes sense. I'm able to merge so once you're happy with it give me a ping and I can get it landed :)

@gfellerph
Copy link
Contributor Author

Thanks for writing this up! I'd like to see more use cases and examples to justify the track, trackslide, and thumb elements. If it turns out that we can do everything we want without the need for those elements, then maybe it would be possible to build it on the newly proposed switch which extends the input element: whatwg/html#9546 Otherwise, we would need to create a separate element.

I agree with mason that we can merge this now regardless

@josepharhar I added a section that compares the two proposals (https://deploy-preview-785--open-ui.netlify.app/components/switch.explainer/#comparison-switch-attribute-vs-switch-element).

@mfreed7 mfreed7 added the switch Switch control label Feb 21, 2024
@mfreed7
Copy link
Collaborator

mfreed7 commented Feb 21, 2024

Seems like this proposal should get landed. Nothing stops further iteration on the explainer, but with it here in PR form, nobody can see it.

@mfreed7
Copy link
Collaborator

mfreed7 commented Feb 21, 2024

Seems like this proposal should get landed. Nothing stops further iteration on the explainer, but with it here in PR form, nobody can see it.

@gfellerph I'm happy to click the button to do that - just give me the go-ahead.

@mfreed7 mfreed7 merged commit ed9de34 into openui:main Feb 29, 2024
5 checks passed
@gfellerph gfellerph deleted the switch-proposal branch February 29, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
switch Switch control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants