Skip to content

Conversation

@jjenzz
Copy link
Contributor

@jjenzz jjenzz commented Aug 18, 2020

There are some significant changes to Label here for discussion. I remembered that it is invalid markup to have multiple form controls in a label (our Checkbox and Switch which render an input and a button would be invalid), so:

  • I have made it a span by default with a role="label" attribute. It still does the aria-labelledby stuff so is read out fine in SRs.
  • If someone passes an htmlFor attribute, it will switch to a label under the hood.
  • If someone passes an as attribute, it will use whatever they pass.

Because this is now a span by default I added a click handler that will click/focus the element ref passed to the label context hook. This means we can make the label click/focus the button in these controls instead of the hidden input.

Using a span also allows us to nest labels which is useful for things like this RadioGroup where the group should have an aria-labelledby and each radio can also be labelled:

<Label>
  Favourite pet
  <RadioGroup as={Root} defaultValue="1">
    <Label>
      <RadioGroup.Item as={Item} value="1">
        <RadioGroup.Indicator as={Indicator} />
      </RadioGroup.Item>
      Cat
    </Label>{' '}
    <Label>
      <RadioGroup.Item as={Item} value="2">
        <RadioGroup.Indicator as={Indicator} />
      </RadioGroup.Item>
      Dog
    </Label>{' '}
    <Label>
      <RadioGroup.Item as={Item} value="3">
        <RadioGroup.Indicator as={Indicator} />
      </RadioGroup.Item>
      Rabbit
    </Label>
  </RadioGroup>
</Label>

I like this because it means that if you want to label something, you can just do it without worrying about structure.

@jjenzz jjenzz force-pushed the reset-stories-radio branch from fe7ef63 to 57246ca Compare August 18, 2020 22:31
Base automatically changed from style-typing to main August 21, 2020 15:10
@jjenzz jjenzz force-pushed the reset-stories-radio branch 2 times, most recently from 2ad253e to b5008c8 Compare August 25, 2020 18:12
Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

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

I feel like the radio package isn't really useful on its own if we're going to offer a higher level API like radio-group.

@jjenzz jjenzz requested a review from benoitgrelard August 26, 2020 16:33
@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 26, 2020

@benoitgrelard I've moved Radio into the RadioGroup package instead

@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 28, 2020

I need to reintroduce Box here too... will do that now

@jjenzz jjenzz force-pushed the reset-stories-radio branch 2 times, most recently from 6f5d35f to 15213fb Compare August 28, 2020 12:26
@jjenzz
Copy link
Contributor Author

jjenzz commented Aug 28, 2020

This is ready for you again @benoitgrelard ❤️

@jjenzz jjenzz force-pushed the reset-stories-radio branch 2 times, most recently from 6801cc7 to d5d2f95 Compare September 1, 2020 18:36
@jjenzz
Copy link
Contributor Author

jjenzz commented Sep 2, 2020

After a quick chat with @benoitgrelard , We've agreed to rethink the Checkbox and Radio APIs slightly and try to avoid exposing the Input since it is an implementation detail that is causing many DX issues.

@jjenzz jjenzz marked this pull request as draft September 2, 2020 10:38
@chaance
Copy link
Member

chaance commented Sep 2, 2020

I'm going to hold on reviewing this one until we get Checkbox merged for all the reasons you mentioned. Once we sort some of those things out, this one should be a lot simpler.

@jjenzz jjenzz force-pushed the reset-stories-radio branch from d5d2f95 to 1087d4d Compare September 24, 2020 17:50
@jjenzz jjenzz changed the title Reset stories for Radio Add RadioGroup Sep 24, 2020
@jjenzz jjenzz force-pushed the reset-stories-radio branch from 1087d4d to 096d363 Compare September 24, 2020 18:02
@jjenzz jjenzz changed the base branch from main to switch-refactor September 24, 2020 18:03
@jjenzz jjenzz force-pushed the reset-stories-radio branch 3 times, most recently from d606529 to 3a53a1f Compare September 24, 2020 18:15
...groupProps
} = props;
const labelId = useLabelContext();
const labelledBy = ariaLabelledby || labelId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vercel vercel bot temporarily deployed to Preview October 2, 2020 14:05 Inactive
@jjenzz jjenzz force-pushed the reset-stories-radio branch from b81f103 to 2056739 Compare October 2, 2020 14:06
@vercel vercel bot temporarily deployed to Preview October 2, 2020 14:06 Inactive
@jjenzz jjenzz force-pushed the reset-stories-radio branch from 2056739 to ae961b8 Compare October 2, 2020 14:06
@jjenzz jjenzz force-pushed the reset-stories-radio branch from ae961b8 to dbb287b Compare October 2, 2020 14:10
@chaance
Copy link
Member

chaance commented Oct 5, 2020

@jjenzz Just to catch me up, is this ready for me to review now? I see @benoitgrelard has approved already.

@benoitgrelard
Copy link
Contributor

@chaance Yes it is, we wanted to wait for you to have a look at it.

Copy link
Member

@chaance chaance left a comment

Choose a reason for hiding this comment

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

Minor changes requested then GTG 👍

@jjenzz
Copy link
Contributor Author

jjenzz commented Oct 5, 2020

@chaance I think I've resolved your feedback but may have misunderstood 🙈 let me know if there's more you're hoping for here.

@jjenzz jjenzz requested a review from chaance October 5, 2020 19:31
@jjenzz jjenzz force-pushed the reset-stories-radio branch from c4d807b to 47e5e46 Compare October 6, 2020 16:08
@jjenzz jjenzz merged commit 5ecaaec into main Oct 6, 2020
@jjenzz jjenzz deleted the reset-stories-radio branch October 6, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants