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

[feature] Pass rest attributes to radio group items #2313

Merged

Conversation

LucasCalazans
Copy link
Member

@LucasCalazans LucasCalazans commented Apr 5, 2020

Description

Forwarding additional attributes to Radio component using RadioGroup.

Related Issue

Closes #2270.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Use the RadioGroup on a page.
  2. Pass other attributes in the item object like this:
<RadioGroup items={[
    {
        value: 'value 1',
        label: 'label 1',
        disabled: true
    },
    {
        value: 'value 2',
        label: 'label 2',
        role: 'radio'
    }
]}/>
  1. Check the new attributes rendered in the page. The first should be disabled the second should be enabled.

Screenshots / Screen Captures

image

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Apr 7, 2020

Fails
🚫 A version label is required. A maintainer must add one.
Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 3abb3c8

sirugh
sirugh previously requested changes Apr 14, 2020
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

We've converted this component into a functional component but your changes should still apply. Please consider my comment and resolve the merge conflict :)

packages/venia-ui/lib/components/RadioGroup/radioGroup.js Outdated Show resolved Hide resolved
@sirugh sirugh changed the title Using rest to pass props to radio item Pass disabled attribute to radio group items Apr 20, 2020
@sirugh sirugh changed the title Pass disabled attribute to radio group items [feature] Pass disabled attribute to radio group items Apr 20, 2020
@sirugh sirugh changed the title [feature] Pass disabled attribute to radio group items [feature] Pass rest attributes to radio group items Apr 20, 2020
tjwiebell
tjwiebell previously approved these changes Apr 20, 2020
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Consider adding a unit test before @dpatil-magento notices.

@@ -23,6 +23,18 @@ stories.add('With a message', () => {
return <RadioGroup items={items} message={'I am a message.'} />;
});

stories.add('with disabled items', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Signed-off-by: sirugh <[email protected]>
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

If the first six label/value pairs didn't catch a bug, surely the seventh will 😂Thanks for adding a test.

@sirugh sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Apr 20, 2020
@dpatil-magento dpatil-magento merged commit d849c28 into magento:develop Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI_APPROVED This PR should be included in the CI process. Event: Global-Contribution-Day Groomed for GCD 2020 Partner: Webjump partners-contribution pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: RadioGroup cannot pass a disabled attribute to the Radio component
6 participants