#59: Add Fieldset component #60

ynotdraw · 2023-03-03T14:59:33Z

🚀 Description

This PR introduces a Fieldset component so we can build checkbox and radio groups.

NOTE: These components are not 100% styled yet, as we are still figuring out where to land on a few things. This is about ~75% close though.

Closes #59

🔬 How to Test

Visit https://bf1e003c.ember-toucan-core.pages.dev/docs/components/fieldset

📸 Images/Videos of Functionality

a11y

Show how the screenreader picks up on the hint + error text

fieldset-a11y.mov

changeset-bot · 2023-03-03T14:59:36Z

⚠️ No Changeset found

Latest commit: 4bcda86

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-03T15:03:55Z

Preview URLs

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

ynotdraw · 2023-03-03T17:09:07Z

docs/components/fieldset/demo/base-demo.md

+>
+  <p class='text-body-and-labels text-xs m-0 italic'>~Fieldset components render
+    here!~</p>
+</Form::Fieldset>


Essentially how some of this will look with the checkbox group is:

<Form::Fieldset @label='Label' @hint='Extra information about the fieldset' @error='Error message' > <Form::CheckboxField @label='Label1' name="label1" /> <Form::CheckboxField @label='Label2' name="label2" /> <Form::CheckboxField @label='Label3' name="label3" /> </Form::Fieldset>

Maybe I should render this instead of the <p> tag? I was hoping to convey this can be used as a generic fieldset component, but I'm not sure which is better for an example! Thoughts?

A demo for checkboxes and radios (maybe as a follow up) would be great! It seems like the most common use case from what you mentioned.

wait...do we only get one demo per component? If think so...if so yeah ^^ I would make those changes.

in the default docfy setup (which this is closer to, as opposed to the CS-internal docfy usage), additional demos are appended on the page vertically -- example here: https://docfy.dev/docs/ember/components/docfy-previous-and-next-page
folder for the demos here: https://github.com/josemarluedke/docfy/tree/main/packages/ember/docs/components/docfy-previous-and-next-page-demo

I would also appreciate a checkbox demo, however the default should IMHO be as-is, to not confuse users about the different concerns of these components.

This PR should make our use of demos more flexible: josemarluedke/docfy#152

simonihmig · 2023-03-06T10:06:31Z

docs/components/fieldset/demo/base-demo.md

+>
+  <p class='text-body-and-labels text-xs m-0 italic'>~Fieldset components render
+    here!~</p>
+</Form::Fieldset>


I would also appreciate a checkbox demo, however the default should IMHO be as-is, to not confuse users about the different concerns of these components.

This PR should make our use of demos more flexible: josemarluedke/docfy#152

simonihmig · 2023-03-06T10:16:18Z

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

+      <Fieldset @label="Label" @isDisabled={{true}} data-fieldset />
+    </template>);
+
+    assert.dom('[data-fieldset]').isDisabled();


This is fine. But I believe this will only prevent any inputs submitting their value for "normal" forms not handled by JS. For this PR it doesn't matter, as these atoms here don't handle forms or form submissions at all. But at the form level we need to make sure this works as expected. Created this for headless: CrowdStrike/ember-headless-form#73

Sounds great to me. Thanks a bunch for filing that!

simonihmig · 2023-03-06T10:18:28Z

docs/components/fieldset/index.md

+
+## Label
+
+Provide a string to `@label` to render the text into the `<legend>` of the Fieldset. This is required.


I wonder if there might be use cases where you would want to not have a label, or rather have the label be visually hidden (but present for screen readers)?

If so, does not need to be as part of this PR, can still add this later if needed...

Totally! I think we will definitely need this quite soon for all form components. Went ahead and filed #62

Think a @isLabelHidden argument would do? Maybe a different name? Or maybe a different way to solve this completely?

Yeah, agree, something like that! I would still make @label mandatory, to enforce a11y.

to enforce a11y.

Oh absolutely! Nice!

ynotdraw self-assigned this Mar 3, 2023

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

Create Input Group component #59

Closed

ynotdraw changed the title ~~feat: Add Fieldset component~~ #59: Add Fieldset component Mar 3, 2023

ynotdraw force-pushed the 59-create-input-group-component branch from c3ab9f1 to 626d095 Compare March 3, 2023 16:12

feat: Add Fieldset component

4bcda86

ynotdraw force-pushed the 59-create-input-group-component branch from 626d095 to 4bcda86 Compare March 3, 2023 17:02

ynotdraw commented Mar 3, 2023

View reviewed changes

ynotdraw marked this pull request as ready for review March 3, 2023 17:09

ynotdraw requested review from nicolechung, noahgelmancs, simonihmig, danwenzel and joelamb March 3, 2023 17:10

nicolechung approved these changes Mar 3, 2023

View reviewed changes

simonihmig approved these changes Mar 6, 2023

View reviewed changes

ynotdraw merged commit 1f9b444 into main Mar 6, 2023

ynotdraw deleted the 59-create-input-group-component branch March 6, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#59: Add Fieldset component #60

#59: Add Fieldset component #60

ynotdraw commented Mar 3, 2023 •

edited

Loading

changeset-bot bot commented Mar 3, 2023 •

edited

Loading

github-actions bot commented Mar 3, 2023 •

edited

Loading

ynotdraw Mar 3, 2023 •

edited

Loading

nicolechung Mar 3, 2023

nicolechung Mar 3, 2023

NullVoxPopuli Mar 3, 2023

simonihmig Mar 6, 2023

simonihmig Mar 6, 2023

simonihmig Mar 6, 2023

ynotdraw Mar 6, 2023

simonihmig Mar 6, 2023

ynotdraw Mar 6, 2023

simonihmig Mar 6, 2023

ynotdraw Mar 6, 2023


		## Label

		Provide a string to `@label` to render the text into the `<legend>` of the Fieldset. This is required.

#59: Add Fieldset component #60

#59: Add Fieldset component #60

Conversation

ynotdraw commented Mar 3, 2023 • edited Loading

🚀 Description

🔬 How to Test

📸 Images/Videos of Functionality

a11y

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

⚠️ No Changeset found

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

Preview URLs

ynotdraw Mar 3, 2023 • edited Loading

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

ynotdraw commented Mar 3, 2023 •

edited

Loading

changeset-bot bot commented Mar 3, 2023 •

edited

Loading

github-actions bot commented Mar 3, 2023 •

edited

Loading

ynotdraw Mar 3, 2023 •

edited

Loading