Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CheckboxGroup with more than one Text component that has unique id-props set causes an infinite React re-render loop #6229

Open
oscarcarlstrom opened this issue Apr 18, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@oscarcarlstrom
Copy link

oscarcarlstrom commented Apr 18, 2024

Provide a general summary of the issue here

It seems like if you put more than one <Text>-component that has an id prop set inside a <CheckboxGroup>, React gets stuck in an infinite re-render loop.

This simple example causes infinite re-renders:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text id="first" slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
      <Text id="second" slot="description">
        Choose this option to choose the second one
      </Text>
    </CheckboxGroup>

But if you remove the id-prop it works fine:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
      <Text slot="description">
        Choose this option to choose the second one
      </Text>
    </CheckboxGroup>

Using only one Text component also works fine:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text id="first" slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
    </CheckboxGroup>

🤔 Expected Behavior?

React should not re-render, since no prop seemingly changes (the id prop is constant, and never changes)

😯 Current Behavior

React re-renders until it reaches the max update depth.

💁 Possible Solution

No response

🔦 Context

Would like to use the Text-component inside the CheckboxGroup to provide descriptions related to the options. These could also contain links to something like terms and conditions, or similiar (which can not, and should not be part of the Checkbox itself): issue 1166. Setting the id props can help screen readers tie it to the correct Checkbox using aria-describedby .

🖥️ Steps to Reproduce

Simple example with no styles (open the console to see multiple Warning: Maximum update depth exceeded.) https://codesandbox.io/p/sandbox/divine-bush-hmfqgv

Version

1.1.1

What browsers are you seeing the problem on?

Firefox, Chrome

If other, please specify.

No response

What operating system are you using?

macOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@LFDanLu
Copy link
Member

LFDanLu commented Apr 19, 2024

So at the moment, the CheckboxGroup only supports a single Text element with slot="description" since said Text element serves as the descriptor for the CheckboxGroup as well. I imagine there is some logic in useCheckboxGroup that is getting confused by the existence of two distinct ids that it is supposed to link the CheckboxGroup to thus causing it to switch from one to the other over and over again.

For now you could maybe use non-slotted text elements and pass their ids as aria-describedbys to the Checkboxes, not something we officially support though. We'll be looking into adding support for HelpText (aka descriptions) to individual Checkboxes via #6192, perhaps making this easier to setup

@LFDanLu LFDanLu added the enhancement New feature or request label Apr 19, 2024
@oscarcarlstrom
Copy link
Author

Thanks @LFDanLu, yeah I noticed that the <Text/> component requires the slot prop, could bypass TypeScript of course. But I ended up just wrapping the content of the <Text/> component in a div, and put the id on that element instead. Works fine, but would be nice to get rid of that extra element. Looking forward to the <HelpText/> component!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants