#67: Add radio + radiogroupfield component #69

ynotdraw · 2023-03-07T19:29:30Z

🚀 Description

This PR introduces a Radio control component, a RadioField component, and a RadioGroupField component. It follows the same pattern as Checkbox + CheckboxField: #50

Closes #67

🔬 How to Test

View https://25c29909.ember-toucan-core.pages.dev/docs/components/radio
View https://25c29909.ember-toucan-core.pages.dev/docs/components/radio-field
View https://cd7b4e7b.ember-toucan-core.pages.dev/docs/components/radio-group-field
- NOTE: These components are not 100% styled yet, as we are still figuring out where to land on a few things. This is about ~90% close though.

📸 Images/Videos of Functionality

a11y test

NOTE: It's saying 14 radio buttons as I am rendering all available UI states below so it's counting all of those 🙈

Screen.Recording.2023-03-07.at.2.30.46.PM.mov

changeset-bot · 2023-03-07T19:29:33Z

⚠️ No Changeset found

Latest commit: 80c4334

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2023-03-07T19:39:53Z

Preview URLs

GH Env: preview
docs: https://131e8b3c.ember-toucan-core.pages.dev

ynotdraw · 2023-03-07T19:53:39Z

docs/components/radio-field/demo/base-demo.md

+  eq = (value, selectedValue) => {
+    return value === selectedValue;
+  };


Local helper functions are a game changer!

Absolutely! 💯

Will even get easier to use with template-imports, as you don't need to assign them to the backing class, you can define the function in the module, or just import it form elsewhere. 🎉

Historically in Ember we needed a name for this primitive, so we have "helper". But actually we now could just call them "a function". There is nothing special anymore (except for when you need a class-based helper though)

ember-toucan-core/src/components/form/radio-field.hbs

simonihmig

Code-wise this looks good! But...

I have a bit of a problem or question wrt to what we define as the "field" here. At least the radio field does not map to what a radio field is in headless-form. Which does not mean the one is "right" and the other is "wrong", but at least we should see if/how we can align this maybe and fill the gap...

Take this example, where we have a form with three fields (in the sense that I have been using that term): name, email and gender. So "field" is basically an element of the form that the user can assign (enter or select) a value to. And it has a @name that will map to the form data object. What is inherently special about a radio group field here is that a single selection (here "gender") is represented by multiple radio inputs, unlike all the other controls where this is a 1:1 mapping. You could represent the gender selection as well using a single <select> control. But in this sense I would see the group of multiple radio buttons as the "field". Having a selection with a single radio button makes no sense after all.

This is different to what we have here, as here a "radio field" represents a single radio button (and related label and stuff).

So one thing is the naming here, which we probably can change easily. But also I wonder if we need something that represents that "radio group" as a whole (the PR title even mentions "radiogroupfield", while I don't see that here, right?). So something that matches what field.Radio does in headless-form. Off the top of my head, that one could:

yield a contextual .RadioField component (the one we already have here)
accept a single @onChange and curry that to the yielded .RadioField
accept a @value which is not the value of the single radio button, but basically the user-selected value. This would match what e.g. an InputField would have, as that also has @value representing the entered value. We don't have an equivalent to that here yet AFAICT, right?
apply the eq logic ootb, as it has the selected @value as well as the value of each individual radio button (yielded RadioField)
apply the same name to each radio button.

Regarding the last point of name, this opens another can of worms. So first, assigning a name to each form control is not only best-practice for semantic form markup (as that is how forms work without JS), it is also a requirement (or strong recommendation at least) for using this with headless-form (you probably remember those discussions around triggering validations and captureEvent in case the control has no name).

For radio groups that is especially important, as the common name basically defines what the group is, so you get the mutually exclusive behaviour of a radio group. With our JS-based solution, we can actually get around that, as we do those eq checks in JS, but still I would consider it strongly recommended to have a name applied. I believe that could also actually fix the screen reader problem you were seeing, as when the radio buttons have no name they share, then there is no way for the user agent to determine what the group is they belong to!

So, with a plain input, we can just pass the name as an attribute as in <InputField name="foo"/>, but this wouldn't work for a radio group as in <RadioGroup name="foo">, because we would need to spread that name to all individual radio inputs in that group, and you cannot do that with HTML attributes (there is only a singe ...attributes). headless-form uses an @name argument, which it can "curry" to all radio inputs. Maybe we have to introduce that argument here also? (and for consistency across all other fields)

simonihmig · 2023-03-08T12:03:24Z

docs/components/radio-field/demo/base-demo.md

+  eq = (value, selectedValue) => {
+    return value === selectedValue;
+  };


Absolutely! 💯

Will even get easier to use with template-imports, as you don't need to assign them to the backing class, you can define the function in the module, or just import it form elsewhere. 🎉

Historically in Ember we needed a name for this primitive, so we have "helper". But actually we now could just call them "a function". There is nothing special anymore (except for when you need a class-based helper though)

ember-toucan-core/src/components/form/radio-field.hbs

test-app/tests/integration/components/radio-test.gts

ynotdraw · 2023-03-08T17:27:30Z

@simonihmig Thanks a bunch for your thoughts! I definitely agree here. I was thinking at first that this could be the beginnings of the RadioGroup/RadioGroupField component, but then decided to punt on it until we get more forms stuff wired up. However, after this discussion, I 100% agree that we should handle this here and now, as part of this PR as it'll make our lives easier to solve this now.

Based on above, your comments, and thinking out loud maybe we can do this:

Keep RadioField as it is now. It essentially combines Field + Radio components
- 100% open to renaming it to something else if it makes sense tho!
Add RadioGroup/RadioGroupField (any name preference?) which will handle all of the logic in your bullet points above
- Not sure about the name - adding Field to the end would mean that it's a proper "field" definition as you describe above though, so maybe that's better?

So:

yield a contextual RadioField component (the one we already have here)

accept a single @onChange and curry that to the yielded .RadioField

accept a @value which is not the value of the single radio button, but basically the user-selected value. This would match what e.g. an InputField would have, as that also has @value representing the entered value. We don't have an equivalent to that here yet AFAICT, right?

apply the eq logic ootb, as it has the selected @value as well as the value of each individual radio button (yielded RadioField)

apply the same name to each radio button

Some rough pseudocode maybe:

{{!-- RadioGroup / RadioGroupField --}}

<Form::Fieldset
  @label={{@label}}
  @hint={{@hint}}
  @error={{@error}}
>
    {{yield (hash RadioField=(ensure-safe-component this.RadioField) name=@name onChange=this.handleChange isSelected={{this.checkRadioGroupValueVersusThisRadiosValue)}}
</Form::Fieldset>

And then usage of above would be something like:

<RadioGroup @label="Label" @name="options" @value="option-1" @onChange={{this.radioGroupChange}} as |group|>
  <group.RadioField @label="Opt 1" @value="option-1" />
  <group.RadioField @label="Opt 2" @value="option-2" />
  <group.RadioField @label="Opt 3" @value="option-3" />
</RadioGroup>

What do you think?

Whatever we decide here, I'll also apply to the checkbox group. It's a little different but has similar issues!

simonihmig · 2023-03-08T17:59:54Z

What do you think?

Looks perfect to me! Except for...

Not sure about the name

... naming things. 🙈

So I am tending more towards also giving this a ...Field name, given I see a field as mentioned already as an element of the form that a user can assign a value to, which also means it has a label and maybe more. Which still matches what we have here, right?

Related to that: what about hints and errors, they will show up primarily for the RadioGroup(Field), right? Will they also be applicable to the child RadioFields? The current API allows that, but as you mentioned yourself (wrt to errors) it's a bit questionable whether that will actually be used, right?

So in that regard also, the RadioGroup(Field) has most of the stuff (label, hint, error) that all the other fields also have, so even more reason to call it a field!?

ynotdraw · 2023-03-08T18:09:23Z

... naming things. 🙈

So true, ugh. ChatGPT to solve naming when!? 😂

Related to that: what about hints and errors, they will show up primarily for the RadioGroup(Field), right? Will they also be applicable to the child RadioFields? The current API allows that, but as you mentioned yourself (wrt to errors) it's a bit questionable whether that will actually be used, right?

Yeah! So we do have a case where an individual RadioField will have a Label + Hint text today. It sits with the radio itself like this:

But also at the RadioGroupField / <fieldset> + <legend> level we also have a Label + Hint:

So we kind of have two labels + two hints:

At the RadioGroupField level
At the RadioField

As for errors though, based on current designs, that should live only in RadioGroupField, as individual RadioFields do not have errors there.

ynotdraw · 2023-03-09T14:56:25Z

docs-app/package.json

-    "@glint/core": "^0.9.6",
-    "@glint/environment-ember-loose": "^0.9.6",
-    "@glint/environment-ember-template-imports": "^0.9.6",
-    "@glint/template": "^0.9.6",
+    "@glint/core": "^1.0.0-beta.3",
+    "@glint/environment-ember-loose": "^1.0.0-beta.3",
+    "@glint/environment-ember-template-imports": "^1.0.0-beta.3",
+    "@glint/template": "^1.0.0-beta.3",


Turns out these were clobbering the builds. See https://github.com/CrowdStrike/ember-toucan-core/actions/runs/4375107381/jobs/7655418852 for the errors we get without updating these

How did you figure this out that it needed updating?!

Stumbled around for a bit longer than I care to admit ( 🙈 ), BUT!

test-app lint: [lint:types] ../node_modules/.pnpm/@glint+template@0.9.7_@glimmer+component@1.1.2/node_modules/@glint/template/-private/integration.d.ts:21:64

I noticed 0.9.7 in the error log from the link above. So I did a search of @glint/template and noticed docs-app was the only one not on beta-3. Updated it and it all worked!

good to know!

ynotdraw · 2023-03-09T14:58:46Z

Looks like the cloudflare action is failing because of cloudflare/pages-action#66 👀

Maybe I should update to cloudflare/pages-action@v1.4.0?

NullVoxPopuli · 2023-03-09T15:00:23Z

Maybe I should update to cloudflare/pages-action@v1.4.0?

I just did that an hour or so ago on ember-resources, can confirm it works. 🎉

ynotdraw · 2023-03-09T15:05:47Z

Thanks @NullVoxPopuli ! 🙇

ynotdraw · 2023-03-09T20:27:50Z

Okay this should be all good to review now!

See CrowdStrike/ember-toucan-core#69 (comment)

simonihmig

Beautiful! 😍

One note to leave here though: it seems CrowdStrike/ember-headless-form#22 also applies here to some extend, right? Like at least the point of where to apply aria-invalid is also not solved here AFAICT, right?

For aria-describedby, this is now handled by the fieldset component here, so aria-describedby is attached to the <fieldset> in case of errors, correct?

simonihmig · 2023-03-10T08:58:26Z

docs/components/radio-group-field/demo/base-demo.md

+    @value='option-4'
+    @hint='Extra information about the radio'
+  />
+</Form::RadioGroupField>


Yeah, that API looks 🤩

simonihmig · 2023-03-10T09:05:08Z

ember-toucan-core/src/components/form/radio-field.ts

+    /**
+     * This component argument is used to determine if the underlying radio is checked.
+     * When `selectedValue` and `value` are equal, the radio will have the checked attribute applied.
+     */
+    selectedValue?: string;


It would be pretty much pointless to try to disagree here, given that this is exactly what I am doing in headless here and here 😅

simonihmig · 2023-03-10T09:08:11Z

ember-toucan-core/src/components/form/radio-group-field.hbs

+        name=@name
+        onChange=this.handleInput
+        selectedValue=@value
+        isDisabled=@isDisabled


This is also nice to have by having a group, to disable all radio buttons at once! 🚀

simonihmig · 2023-03-10T09:12:52Z

ember-toucan-core/src/components/form/radio-group-field.ts

+export interface ToucanFormRadioGroupFieldComponentSignature {
+  Element: HTMLFieldSetElement;
+  Args: {
+    error?: string;


Doesn't have to be in this PR, but just remembered @nicolechung mentioning this in one of her reviews of some headless PR: would be nice to have some JSDoc comments here explaining the args in a short sentence. As this is what people will see when having Glint enabled. And probably they will more likely look only at that, and not consult the actual docs, as we all know how docs get (not) read... 😅

Created: #73

simonihmig · 2023-03-10T09:16:48Z

test-app/tests/integration/components/radio-group-field-test.gts

+import RadioGroupField from '@crowdstrike/ember-toucan-core/components/form/radio-group-field';
+import { setupRenderingTest } from 'test-app/tests/helpers';
+
+module('Integration | Component | RadioGroupField', function (hooks) {


Nice test coverage here!

nicolechung · 2023-03-13T17:01:01Z

ember-toucan-core/src/components/form/radio-group-field.hbs

+        onChange=this.handleInput
+        selectedValue=@value
+        isDisabled=@isDisabled
+      )


Suggestion: Shouldn't we also yield the selectedValue in case the user wants to format that in some way?

NVM - read the thread above!

ynotdraw · 2023-03-13T17:45:39Z

One note to leave here though: it seems CrowdStrike/ember-headless-form#22 also applies here to some extend, right? Like at least the point of where to apply aria-invalid is also not solved here AFAICT, right?

Yup yup, that's correct. Whatever @noahgelmancs finds there I'll apply here as well!

For aria-describedby, this is now handled by the fieldset component here, so aria-describedby is attached to the
in case of errors, correct?

Yup that's correct! Will need to verify that is okay as well!

ynotdraw self-assigned this Mar 7, 2023

ynotdraw linked an issue Mar 7, 2023 that may be closed by this pull request

Add RadioGroupField component #67

Closed

ynotdraw changed the title ~~[WIP] #67: Add radio + radiogroupfield component~~ #67: Add radio + radiogroupfield component Mar 7, 2023

ynotdraw commented Mar 7, 2023

View reviewed changes

ynotdraw marked this pull request as ready for review March 7, 2023 20:06

ynotdraw requested review from nicolechung, simonihmig, noahgelmancs, danwenzel and joelamb and removed request for nicolechung March 7, 2023 20:06

simonihmig reviewed Mar 8, 2023

View reviewed changes

nicolechung reviewed Mar 8, 2023

View reviewed changes

test-app/tests/integration/components/radio-test.gts Show resolved Hide resolved

nicolechung reviewed Mar 8, 2023

View reviewed changes

test-app/tests/integration/components/radio-test.gts Show resolved Hide resolved

ynotdraw force-pushed the 67-add-radiogroupfield-component branch from 16882aa to 8e4deb4 Compare March 8, 2023 17:59

ynotdraw marked this pull request as draft March 8, 2023 18:13

ynotdraw changed the title ~~#67: Add radio + radiogroupfield component~~ [WIP] || #67: Add radio + radiogroupfield component Mar 8, 2023

ynotdraw commented Mar 9, 2023

View reviewed changes

ynotdraw changed the title ~~[WIP] || #67: Add radio + radiogroupfield component~~ #67: Add radio + radiogroupfield component Mar 9, 2023

github-actions bot deployed to ember-toucan-core (Preview) March 9, 2023 15:08 View deployment

ynotdraw marked this pull request as ready for review March 9, 2023 15:16

ynotdraw added 17 commits March 9, 2023 14:40

fix: dependency issue due to linking

c3fac18

fix: Mark value as required

3d14aaf

fix: update assertion messages

35063e1

fix: Remove error from RadioField

869dbeb

tests: Add another classes check

739b6ce

feat: Add ensure-safe-component helper type

9c92ef6

feat: Add default spacing in fieldset controls

f120d12

feat: Add RadioGroupField component

b104352

feat: Remove isChecked in favor of selectedValue

6536f13

feat: Add padding for fieldset controls for error

7b2d5f2

docs: Add docs for radio-group-field

30307bf

tests: Add radio-group-field tests

d96a7cc

feat: Make name required

bc670ca

feat: Curry isDisabled from Group to children

f3ec59f

deps: Update to glint beta-3

778817d

docs: Use different names

948e8ce

docs: Add value docs to radio-field

80c4334

ynotdraw force-pushed the 67-add-radiogroupfield-component branch from 438fa19 to 80c4334 Compare March 9, 2023 19:41

github-actions bot deployed to ember-toucan-core (Preview) March 9, 2023 19:44 View deployment

simonihmig added a commit to CrowdStrike/ember-headless-form that referenced this pull request Mar 10, 2023

Fix cloudflare action

a2c01ac

See CrowdStrike/ember-toucan-core#69 (comment)

simonihmig mentioned this pull request Mar 10, 2023

Fix cloudflare action CrowdStrike/ember-headless-form#79

Merged

simonihmig approved these changes Mar 10, 2023

View reviewed changes

nicolechung reviewed Mar 13, 2023

View reviewed changes

nicolechung approved these changes Mar 13, 2023

View reviewed changes

ynotdraw merged commit ac2f308 into main Mar 13, 2023

ynotdraw deleted the 67-add-radiogroupfield-component branch March 13, 2023 17:46

simonihmig mentioned this pull request Mar 13, 2023

Improve a11y of radio group CrowdStrike/ember-headless-form#76

Merged

ynotdraw mentioned this pull request Mar 14, 2023

feat: Require RadioGroupField value component argument #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#67: Add radio + radiogroupfield component #69

#67: Add radio + radiogroupfield component #69

ynotdraw commented Mar 7, 2023 •

edited

Loading

changeset-bot bot commented Mar 7, 2023 •

edited

Loading

github-actions bot commented Mar 7, 2023 •

edited

Loading

ynotdraw Mar 7, 2023

simonihmig Mar 8, 2023

simonihmig left a comment

simonihmig Mar 8, 2023

ynotdraw commented Mar 8, 2023 •

edited

Loading

simonihmig commented Mar 8, 2023

ynotdraw commented Mar 8, 2023

ynotdraw Mar 9, 2023

nicolechung Mar 9, 2023

ynotdraw Mar 9, 2023

nicolechung Mar 13, 2023

ynotdraw commented Mar 9, 2023 •

edited

Loading

NullVoxPopuli commented Mar 9, 2023

ynotdraw commented Mar 9, 2023

ynotdraw commented Mar 9, 2023

simonihmig left a comment

simonihmig Mar 10, 2023

simonihmig Mar 10, 2023

simonihmig Mar 10, 2023

simonihmig Mar 10, 2023

simonihmig Mar 10, 2023

nicolechung Mar 13, 2023

nicolechung Mar 13, 2023

ynotdraw commented Mar 13, 2023

#67: Add radio + radiogroupfield component #69

#67: Add radio + radiogroupfield component #69

Conversation

ynotdraw commented Mar 7, 2023 • edited Loading

🚀 Description

🔬 How to Test

📸 Images/Videos of Functionality

a11y test

changeset-bot bot commented Mar 7, 2023 • edited Loading

⚠️ No Changeset found

github-actions bot commented Mar 7, 2023 • edited Loading

Preview URLs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynotdraw commented Mar 8, 2023 • edited Loading

simonihmig commented Mar 8, 2023

ynotdraw commented Mar 8, 2023

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

ynotdraw commented Mar 9, 2023 • edited Loading

NullVoxPopuli commented Mar 9, 2023

ynotdraw commented Mar 9, 2023

ynotdraw commented Mar 9, 2023

simonihmig 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

ynotdraw commented Mar 13, 2023

ynotdraw commented Mar 7, 2023 •

edited

Loading

changeset-bot bot commented Mar 7, 2023 •

edited

Loading

github-actions bot commented Mar 7, 2023 •

edited

Loading

ynotdraw commented Mar 8, 2023 •

edited

Loading

ynotdraw commented Mar 9, 2023 •

edited

Loading