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

fix(10343): Adds support for 'required' prop on RadioTile & TileGroup #16237

Conversation

2nikhiltom
Copy link
Contributor

@2nikhiltom 2nikhiltom commented Apr 22, 2024

Closes #10343

Adds support for 'required' prop on RadioTile
Adds support for 'required' prop on TileGroup

New
RadioTile supports 'required' prop and attaches it to underlying Input
Adds new TestCase to verify this functionality
TileGroup supports 'required' prop and attaches it to children RadioTile

Changed
RadioTile accepts required prop
PublicAPI snapshot updated with 'required' boolean attribute
The required attribute has to be specified only on every TileGroup instead of on every child RadioTile

Testing / Reviewing

  1. Add TileGroup with required RadioTiles to a form like shown below
  2. Submit form without selecting any of the tiles.

(Take pull of this PR and update any one of the storybook example with below code )

export const Test = () => {
  function onSubmit(e) {
    e.preventDefault();
    alert('submitted');
  }

  return (
    <div style={{ padding: '2em' }}>
      <Form onSubmit={onSubmit}>
        <TileGroup name="radio" required>
          <RadioTile value="1">
            Tile 1
          </RadioTile>
          <RadioTile value="2">
            Tile 2
          </RadioTile>
          <RadioTile value="3">
            Tile 3
          </RadioTile>
        </TileGroup>
        <Button type="submit">Submit</Button>
      </Form>
    </div>
  );
};
  1. Verify the repro of below behaviour
image

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 354497a
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66263d9db6d150000805a415
😎 Deploy Preview https://deploy-preview-16237--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit dcf5aad
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/662a6d6650fe16000a47b74b
😎 Deploy Preview https://deploy-preview-16237--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tw15egan
Copy link
Member

tw15egan commented Apr 22, 2024

From the original issue:

The required attribute has to be specified on every RadioTile (in React at least) instead of on the TileGroup

Should required also be added to TileGroup, so you can only set required once instead of on each RadioTile? That way, if a user specifies <TileGroup required />, we can iterate over each RadioTile and set required automatically.

@2nikhiltom
Copy link
Contributor Author

This makes sense, only concern is the proper usage of the HTML element/definition because TileGroup being a fieldset, lacks native support for required. Thoughts ?

@tw15egan
Copy link
Member

@2nikhiltom I wouldn't apply the required to the fieldset, I would just use it to mark each RadioTile as required rather than making the user mark each one individually

@alisonjoseph
Copy link
Member

alisonjoseph commented Apr 22, 2024

@tw15egan if we add required to TileGroup, would we also want to add it to RadioButtonGroup for consistency? (in a separate issue/PR)

@tw15egan
Copy link
Member

Yeah I think it would be good for consistency

@2nikhiltom
Copy link
Contributor Author

2nikhiltom commented Apr 23, 2024

The required attribute has to be specified on every RadioTile (in React at least) instead of on the TileGroup

I have made changes to accommodate the above concern. Now TileGroup can pass required prop , which will internally iterate over each RadioTile and set required automatically.

I will created a separate issue for RadioButtonGroup to maintain consistency. here

@2nikhiltom 2nikhiltom changed the title fix(10343): Adds support for 'required' prop on RadioTile fix(10343): Adds support for 'required' prop on RadioTile & TileGroup Apr 23, 2024
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Looks good to me! I pushed one update here adding tests for the required functionality on TileGroup

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@alisonjoseph alisonjoseph added this pull request to the merge queue Apr 25, 2024
Merged via the queue into carbon-design-system:main with commit 8bb54fa Apr 25, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Native RadioTile validation sub-optimal
4 participants