#587 Add a required attribute to sl-radio-group/sl-radio #595

Trendy · 2021-11-22T22:21:31Z

See #584 for discussion

I still haven't implemented the changes to check the validity in sl-form, just looking for feedback.

vercel · 2021-11-22T22:21:35Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shoelace/shoelace/3Whq4oVZsgiddkntG84sY5D4Qwep
✅ Preview: https://shoelace-git-fork-trendy-radio-group-shoelace.vercel.app

claviska

This is great! I like the idea of considering "radio group" the form control instead of each individual radio. With this PR, we're getting closer to that.

A couple additional observations:

The setCustomValidity() method should also be hoisted to Radio Group.
It would be very convenient if Radio Group worked like Select so you can get/set the selected item using the value prop. For example:

<sl-radio-group label="Select an option">
  <sl-radio value="1" checked>Option 1</sl-radio>
  <sl-radio value="2">Option 2</sl-radio>
  <sl-radio value="3">Option 3</sl-radio>
</sl-radio-group>

<script>
  const radioGroup = document.querySelector('sl-radio-group');

  console.log(radioGroup.value); // outputs "1"

  // The third radio will now be checked
  radioGroup.value = 3;
</script>

src/components/radio-group/radio-group.ts

claviska · 2021-11-22T22:47:42Z

src/components/radio-group/radio-group.ts

+        radios[0].required = true;
+
+        setTimeout(() => {
+          radios[0].reportValidity();


The result of this call to reportValidity() should be the return value in case there's an inconsistency.

I'm assuming the setTimeout was added to wait for the next render so the radio will be ticked, right? All other reportValidity() methods in the library are synchronous, so making this one async feels a bit weird. This might be a good use case for calling update().

Does this work in lieu of the timeout?

radios[0].required = true; const changedProps = new Map<string, any>(); changedProps.set('required', false); radios[0].update(changedProps);

If not, we may have to live with making reportValidity an async method.

The setTimeout was added because I ran into an issue with timing.

I don't know when the sl-radio elements are added to the default slot. So I can't reliably force a required flag on the first radio element during initial render.

I just want the browser-level validation message to display on the first sl-radio element in the group, and the only way to trigger it is to set the underlying InputElement to required, then run the reportValidation. That requires that I manipulate the attribute on sl-radio, which means I need to wait for the event queue to flush before triggering the validation.

There are a couple of things I have thought about to get around this, but haven't decided:

Expose the underlying input element, modify it directly (very dirty IMHO).

Use a custom getter/setter for the required attribute, this would let me directly manipulate the input element before changes have trigger on the sl-radio prop change (which should cause the correct validity message to trigger right after I set the prop on sl-radio).

I could never expose ANY of the underlying input element (itself or attributes) and just provide a triggerRequiredWarning method of some sort. I didn't like this at first, but if you think about a use case where the first input might be off-screen (edge case IMHO)... rl-radio-group could handle that in the future and trigger the validation on a visible input.

Something tired me isn't thinking about.

@Trendy What about making the sl-radio responsible for checking its parent's required state? If a new/first sl-radio is added, it will itself acquire the correct state. If the sl-radio-group receives a (new) required state it can set it on the first sl-radio if present and otherwise assume the first child will pick it up itself.

Expose the underlying input element, modify it directly (very dirty IMHO).

In this case, it's cleaner than forcing an update I guess.

radio.shadowRoot.querySelector('input[type="radio"]').required = true;

It would keep things synchronous, at least. I'm not opposed to it since the components are required to be used together.

docs/components/radio-group.md

Co-authored-by: Cory LaViska <cory@abeautifulsite.net>

Trendy · 2021-11-23T00:32:59Z

src/components/radio-group/radio-group.ts


  @query('slot:not([name])') defaultSlot: HTMLSlotElement;

  /** The radio group label. Required for proper accessibility. Alternatively, you can use the label slot. */
  @property() label = '';

+  /** The current value of the radio group. */
+  @property()
+  get value() {


This might be odd, but I need a way to get the value after render, but not have it be blank because the default isn't in sync with the defaulted 'checked' radio.

If you know a better way to ensure this has a good default when the user does a get on the value.... I'm open to it.

src/components/radio-group/radio-group.ts

Trendy · 2021-11-23T00:36:19Z

src/components/radio-group/radio-group.ts

@@ -39,6 +87,63 @@ export default class SlRadioGroup extends LitElement {
    });
  }

+  getAllRadios(): SlRadio[] {


Should this be documented and exposed to the users?

Nope. They can use radioGroup.querySelector('sl-radio').

Trendy · 2021-11-23T00:36:29Z

src/components/radio-group/radio-group.ts

+    return [...this.querySelectorAll('sl-radio')];
+  }
+
+  checkRadioByIndex(index: number): SlRadio[] {


Should this be documented and exposed to the users?

Nah. They control the slot so it doesn’t seem valuable.

Trendy · 2021-11-23T00:37:14Z

src/components/radio-group/radio-group.ts

+    return radios;
+  }
+
+  deselectAll(): SlRadio[] {


Okay, first copy/pasta was reasonable, but do these methods make sense to expose and document?

Only public API methods should have JSDoc comments. The rest are pseudo-private methods that don’t get published. Is this intended to be public?

ErikOnBike · 2021-11-23T07:56:04Z

src/components/radio-group/radio-group.ts

+      if (index < 0) index = radios.length - 1;
+      if (index > radios.length - 1) index = 0;
+
+      this.getAllRadios().map(radio => (radio.checked = false));


I think forEach would be better than map here.

Is this a performance assumption? I tend to use map over forEach since the result of getAllRadios() is an array.

Nope. The result is not used, so mapping does not add any usefulness. forEach and map both iterate over the array elements. The latter answers a new array with the result of each iteration as new value for the array. Ie [ 1, 2, 3 ].map(function(x) { return x * x; }) will result in [ 1, 4, 9 ]. forEach always returns undefined.

I do think performance is slightly worse with map since a new array structure is created. For forEach this is not necessary. Not sure if this is noticeable in real live.

claviska · 2021-11-23T13:24:27Z

src/components/radio-group/radio-group.ts

@@ -39,6 +42,26 @@ export default class SlRadioGroup extends LitElement {
    });
  }

+  reportValidity() {
+    const radios = [...(this.defaultSlot.assignedElements({ flatten: true }) as SlRadio[])];


For some reason, this results in chunk.QG7EXAZB.js:150 Uncaught TypeError: Cannot read properties of null (reading 'assignedElements') when you click the "Validate group" button.

Any reason you're not using this.getAllRadios() here?

claviska · 2021-11-23T13:31:09Z

src/components/radio-group/radio-group.ts

+        radios[0].required = true;
+
+        setTimeout(() => {
+          radios[0].reportValidity();


Expose the underlying input element, modify it directly (very dirty IMHO).

In this case, it's cleaner than forcing an update I guess.

radio.shadowRoot.querySelector('input[type="radio"]').required = true;

It would keep things synchronous, at least. I'm not opposed to it since the components are required to be used together.

claviska · 2021-11-23T13:40:19Z

src/components/radio-group/radio-group.ts

+  /** Indicates that a selection is required. */
+  @property({ type: Boolean, reflect: true }) required = false;
+
+  connectedCallback() {


Right now, important base class logic that's supposed to happen is being overwritten by this. You need to call super.connectedCallback() at the top of this method. (This is a big gotcha when coming from Stencil to Lit.)

claviska · 2021-11-23T13:40:38Z

src/components/radio-group/radio-group.ts

+    this.addEventListener('sl-change', this.syncRadioButtons);
+  }
+
+  disconnectedCallback() {


Add super.disconnectedCallback() at the top of this method block.

claviska · 2021-11-23T14:58:43Z

I played with this locally and made some improvements. Published them to this branch: https://github.com/shoelace-style/shoelace/tree/Trendy-radio-group

After trying out the updates in VoiceOver, it reads pretty badly. Not sure how to improve on it. We may need to revisit the approach of using Radio Group as a "single control" and make it work more like HTML — but that won't be easy since the inputs are in shadow roots.

This one isn't an easy fix, unfortunately. 😢

claviska · 2021-11-23T16:07:37Z

Sorry for the unintentional merge. I clicked the wrong button in VS Code. 🤦🏻‍♂️

Trendy added 5 commits November 22, 2021 12:34

Add required attribute to radio group and radio

0c6060e

Remove required on sl-radio checks from sl-radio-group

989e368

Clean up react example

dfaabee

Move selection logic from radio to radio group

d4c41a2

Clean up tests

b36d2ed

vercel bot deployed to Preview November 22, 2021 22:21 View deployment

claviska reviewed Nov 22, 2021

View reviewed changes

Update src/components/radio-group/radio-group.ts

2ace7d4

Co-authored-by: Cory LaViska <cory@abeautifulsite.net>

vercel bot had a problem deploying to Preview November 22, 2021 23:10 Failure

Review feedback updates

6eb79aa

vercel bot deployed to Preview November 22, 2021 23:58 View deployment

Fix getter

cfa800c

vercel bot deployed to Preview November 23, 2021 00:18 View deployment

Update react example

3141590

vercel bot deployed to Preview November 23, 2021 00:28 View deployment

Make the default the same as the getter default

e9f7c5e

vercel bot deployed to Preview November 23, 2021 00:30 View deployment

Trendy commented Nov 23, 2021

View reviewed changes

src/components/radio-group/radio-group.ts Show resolved Hide resolved

Trendy commented Nov 23, 2021

View reviewed changes

ErikOnBike reviewed Nov 23, 2021

View reviewed changes

claviska reviewed Nov 23, 2021

View reviewed changes

claviska merged commit e5cbee2 into shoelace-style:next Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#587 Add a required attribute to sl-radio-group/sl-radio #595

#587 Add a required attribute to sl-radio-group/sl-radio #595

Trendy commented Nov 22, 2021

vercel bot commented Nov 22, 2021 •

edited

claviska left a comment

claviska Nov 22, 2021

Trendy Nov 23, 2021

ErikOnBike Nov 23, 2021

claviska Nov 23, 2021

Trendy Nov 23, 2021

Trendy Nov 23, 2021

claviska Nov 23, 2021

Trendy Nov 23, 2021

claviska Nov 23, 2021

Trendy Nov 23, 2021

claviska Nov 23, 2021

ErikOnBike Nov 23, 2021

claviska Nov 23, 2021

ErikOnBike Nov 23, 2021

ErikOnBike Nov 23, 2021

claviska Nov 23, 2021

claviska Nov 23, 2021

claviska Nov 23, 2021

claviska Nov 23, 2021

claviska commented Nov 23, 2021

claviska commented Nov 23, 2021

#587 Add a required attribute to sl-radio-group/sl-radio #595

#587 Add a required attribute to sl-radio-group/sl-radio #595

Conversation

Trendy commented Nov 22, 2021

vercel bot commented Nov 22, 2021 • edited

claviska left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claviska commented Nov 23, 2021

claviska commented Nov 23, 2021

vercel bot commented Nov 22, 2021 •

edited