Ux 603 New Radio component

[tristanjasper]

🛠 Purpose

Add the radio component to Paprika.
http://storybooks.highbond-s3.com/paprika/ux-603-radio-component

Tests to be added in future PR

✏️ Notes to Reviewer

---

🖥 Screenshots

---

[johnpeele]

@tristanjasper You might already be aware, but the checked state for the Large size is a bit off.

[tristanjasper]

@tristanjasper You might already be aware, but the checked state for the Large size is a bit off.

@johnpeele What browser were you seeing this out of interest? I'll look into it.

[nahumzs]

To me sound that this Group radio button should be a uncontrolled component, my concern is that is too much boiler plate when what a consumer pretty much wants it o know which component is checked.

I would approach this API like the following:

<Radio.Group 
   defaultCheck={(check) => check.value.id === "something"} 
   onChange={(value) => { console.log(value) }}
>
   <Radio value={whatever}>First</Radio>
   <Radio value={whatever}>second</Radio>
   <Radio value={whatever}>third</Radio>
<Radio.Group>

This achieve two things one removes the need of any state from the consumer and give the ability to set the initial value via a function that can be used to check which checkbox should be initially selected.

what do you think? @mikrotron @tristanjasper

[nahumzs]

Also this way we are more align the way that most input component works like having a onChange value. like the input, select and pretty much any react component.

[tristanjasper]

Sure that is another option, I guess there isn't really a case for a single radio component to be needed, ie without the surrounding radioGroup container so yeah would be cleaner. I'll move forward with this suggestion

[tristanjasper]

Also this way we are more align the way that most input component works like having a onChange value. like the input, select and pretty much any react component.

Agreed

[johnpeele]

@tristanjasper I am using Brave, which is a version of Chrome.

[nahumzs]

please switch this to a form a function instead of arrow function and destructured the properties on the body.

function Group(props) {
const {a11yLabelledByText, canDeselect, children, defaultCheck, ...moreGroupProps} = props;
}

[tristanjasper]

ok out of interest what is the reason for the preference?

[nahumzs]

💯 thank you 😻

[nahumzs]

@johnpeele We officially support chrome, firefox, Edge, Safari, and we try to do our best to make it render as similar as possible on IE11. I think is quite expensive to try to adjust all known browsers apart from them. cc @mikrotron

[mikrotron]

Hm, how about:

PropTypes.oneOfType([
  PropTypes.bool,
  PropTypes.string,
  PropTypes.number,
  PropTypes.shape({ id: PropTypes.string }),
]).isRequired

[mikrotron]

This would require a bit of logic when you use the value as the key, but gives the consumer more flexibility.

[tristanjasper]

Yep, no longer using the value however as consumer can work with the activeIndex to get the data values

[alexzherdev]

This still assumes Radio is a direct child of Group which defeats the purpose of using context. (Recommend to create a story with code like

<Radio.Group>
  <>
    <Radio />
    <Radio />
  <>
</Radio.Group>

to see that it doesn't work)
If it's acceptable to put the checked status on the group itself (i.e. checkedIndex prop), then I think context can still be used. Otherwise we should revert that change since it's not achieving what we want.

[tristanjasper]

I would have thought it is ok to expect the usage of this component is to always use Radio components as the siblings? I understand that there will be issues if they use a different child but is that a scenario that we need to cover? We do provide storybook examples and a readme to show correct usage... 🤷‍♂

[tristanjasper]

We could throw an error if the wrong child type was passed as well..

[mikrotron]

What if you need side-by-side radios though? You'll need some flexbox divs in there for layout.

[tristanjasper]

Perhaps we will need to revisit the required api in a future PR. I know in the case of formElement we pass an isInline prop to dictate the layout rather than style the children directly, - https://github.com/acl-services/paprika/blob/master/packages/FormElement/src/FormElement.js#L34 But perhaps we are not too keen on that approach? Will need discussion. For now I have changed it back to use cloneElement rather than context. Discussing with @alexzherdev we have established that if we wanted to retain the index to dictate which radio is selected (we are also need to track which radio is deselectable) we would perhaps have to go down the route of https://gist.github.com/ryanflorence/10e9387f633f9d2e6f444a9bddaabf6e#option-4-the-unholy-option.

[mikrotron]

I think we should still use React.Children.map() here, to be a little more defensive.

[mikrotron]

nit: deSelectableIndex → deselectableIndex

[tristanjasper]

Ok, i've changed to getDeselectableIndex as it is a function so I think that is more appropriate

[mikrotron]