shoelace-style / shoelace

A collection of professionally designed, every day UI components built on Web standards. SHOELACE IS BECOMING WEB AWESOME 👇👇👇
https://webawesome.com
MIT License
12.95k stars 831 forks source link

Add ability to focus sl-radio-group dynamically #2192

Closed schilchSICKAG closed 1 month ago

schilchSICKAG commented 1 month ago

Relates to: https://github.com/shoelace-style/shoelace/discussions/2186

This PR adds the ability to call a new focus method on SlRadioGroup. It may be used in the following way:

<sl-radio-group value="1">
  <sl-radio value="0" disabled>Option Disabled</sl-radio>
  <sl-radio value="1">Option 1</sl-radio>
  <sl-radio value="2">Option 2</sl-radio>
</sl-radio-group>

<script>
document
  .querySelector('sl-radio-group')
  .focus();
</script>

The following changes where made to accomplish this:

  1. Moving the logic from handleLabelClick into a new, shared method handleFocus. The logic was already there, but not reusable. I also opted to add another check to the original logic. Previously, it selected the checked item or the first item from getAllRadios. This would have resulted in selecting a potentially disabled option, so I tried to harden this a bit.
  2. Creating a new method focus, which now also accepts the optional FocusOptions, as the other components do and reuses handleFocus.
  3. Creating a bunch of unittests to make sure it works as intended :)

Looking forward to your opinions @claviska @KonnorRogers

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Oct 10, 2024 5:56pm
claviska commented 1 month ago

To avoid confusion, since radio items also have a focus() method and focus() typically assigns focus to the element itself (which we're not doing in this case), we should probably name this focusSelectedItem() or something.

@KonnorRogers @lindsaym-fa what do you think?

KonnorRogers commented 1 month ago

To avoid confusion, since radio items also have a focus() method and focus() typically assigns focus to the element itself (which we're not doing in this case), we should probably name this focusSelectedItem() or something.

@KonnorRogers @lindsaym-fa what do you think?

I see it as no different than delegatesFocus so 🤷 im fine with .focus()

lindsaym-fa commented 1 month ago

To avoid confusion, since radio items also have a focus() method and focus() typically assigns focus to the element itself (which we're not doing in this case), we should probably name this focusSelectedItem() or something.

@KonnorRogers @lindsaym-fa what do you think?

That's a tough call... I'm inclined to agree with @KonnorRogers — focus within a radio group implies focus on the selected item, so I like the simplified semantics of focus(). That said, we offer focus() for <sl-radio-button> already (but not <sl-radio>), and that does muddy things for me.

Is the context of using focus() on <sl-radio-group> vs. <sl-radio-button> enough to recognize that the former focuses the selected item while the latter focuses any specified item? I think so.

claviska commented 1 month ago

Is the context of using focus() on vs. enough to recognize that the former focuses the selected item while the latter focuses any specified item? I think so.

It focuses the selected item OR the first unchecked item, if no selection is currently made. That's why it feels a bit different to me in this case. You can't focus the radio group itself, it will always be passing focus to another element. 😕

schilchSICKAG commented 1 month ago

Calling the method focus makes sure the same shared method names are available on all of shoelaces form elements. It would have been something different when the SlRadio and SlRadioButton where implementing ShoelaceFormControl and are usable in forms out of the box, but they are currently only the visualization for SlRadioGroup which does the heavy lifting.

As a developer building a form, I would expect that methods for the basic form building blocks behave the same.