[WIP] feat(radio-input): add components prop to radio input option #1139

dogayuksel · 2019-10-25T15:33:29Z

Summary

Allows passing in a react element to wrap RadioInput.Option with.

dogayuksel · 2019-10-25T15:38:17Z

#1136

tdeekens

Cool you picked this up so quickly!

tdeekens · 2019-10-25T15:51:12Z

src/components/inputs/radio-input/radio-group.js

@@ -29,7 +29,7 @@ const Group = props => {
  const optionElements = React.Children.map(props.children, (child, index) => {
    // NOTE: Allowing to intersperse other elements than `Option`.
    if (child && child.type.displayName === Option.displayName) {
-      return React.cloneElement(child, {
+      const option = React.cloneElement(child, {


Nit: maybe children as a name would be apt here.

const clonedChild?

tdeekens · 2019-10-25T15:52:47Z

src/components/inputs/radio-input/radio-group.js

@@ -41,6 +41,13 @@ const Group = props => {
        onFocus: props.onFocus,
        onBlur: props.onBlur,
      });
+      return child.props.components.WrapperComponent ? (


Nice! Wondering, as I always struggle to read <a with a lowercase character if it is actually a React component. Would

const Wrapper = child.props.components.WrapperComponent || React.Fragment;

work to then just do:

<Wrapper>{children}</Wrapper>

Hm, another approach we could go for just allows any child but leave it up to the consumer to not allow prop spreading. That way we don't need a new components prop.

So for your case, you would do something like this. Basically capture the props in your wrapper then pass them to the component that should receive them (RadioOption).

const Wrapper = props => { <ToolTip off={isTooltipOff}> <RadioInput.Option {...props} /> <ToolTip> }

I like this self-service approach. Would we be recursively searching for an RadioInput.Option as a child for the invariant, or removing the check all together, what do you think?

I would be fine with removing it. A recursive search could also be misleading. As in: somewhere in the component tree there is a Radio.Option but it wasn't the one the developer but "nearby" his concern. So the invariant will maybe rarely give an appropriate hint.

Hm, I'm not sure how we could do this though without making it a breaking change.

montezume · 2019-10-26T08:44:17Z

src/components/inputs/radio-input/radio-option.js

@@ -66,6 +67,16 @@ Option.propTypes = {
    PropTypes.element,
    PropTypes.func,
  ]).isRequired,
+  components: PropTypes.shape({
+    WrapperComponent: (props, propName) => {


is this really necessary? Can't we just make it optional?

You mean the children prop right?

dogayuksel · 2019-10-28T09:05:11Z

@montezume @tdeekens
Any ideas how to pass in props to WrapperComponent in this setup? For example, wrapper being a Tooltip and user wanting to control the off prop.

Add an other prop to RadioInput.Option, like WrapperComponentProps and spread them on to WrapperComponent?
Otherwise, WrapperComponent can be a JSX.Element but I don't know, if it is possible to append children to that.

tdeekens · 2019-10-28T09:15:31Z

Hm, good question. So far we only swaped out default components while applying "internal" props to them. Correct me if that's wrong @montezume.

A prop-getter like getWrapperComponentProps feels sort of backwards
A props.props.WrapperComponent feels also a bit odd
A props.components.WrapperComponent.props and props.components.WrapperComponent.component also looks like an odd API

Sorry I am just thinking out loud. 🤔 would it make sense just not to render the wrapper component in the parent? It doesn't really solve the "issue" though.

tdeekens · 2019-10-29T16:53:12Z

src/components/inputs/radio-input/radio-option.js

-        return new Error(
-          `Invalid prop 'components.WrapperComponent' supplied to 'RadioInput.Option': the prop is not a valid React component`
-        );
+    wrapper: (props, propName) => {


Wondering. Is this almost the same as PropTypes.oneOf([PropTypes.func, PropTypes.array, PropTypes.elementType])?

Tried to restrict it to a function (1st if) that receives one argument (2nd if) and returns a valid react.element (3rd if)

Maybe a bit over engineered and fails stupidly, thought it could help/guide the user maybe

True. It's a nice idea! Maybe it's also something UIKit could share and reuse internally.

montezume

I played around with it locally. Really nice approach. 🎉

Thanks @dogayuksel

montezume · 2019-10-30T09:39:36Z

@tdeekens do you want to review?

montezume · 2019-10-30T09:59:35Z

Not sure why Percy didn't report ... it passed https://percy.io/commercetools-GmbH/ui-kit/builds/2967506

tdeekens · 2019-10-30T10:02:51Z

src/components/inputs/radio-input/radio-group.js

@@ -41,6 +41,8 @@ const Group = props => {
        onFocus: props.onFocus,
        onBlur: props.onBlur,
      });
+      const { wrapper } = child.props.components;
+      return wrapper ? wrapper(clonedChild) : clonedChild;


This is cool.

feat(radio-input): add components prop to radio input option

beb200b

dogayuksel added the 🚧 Status: WIP Work in progress label Oct 25, 2019

dogayuksel self-assigned this Oct 25, 2019

tdeekens reviewed Oct 25, 2019

View reviewed changes

montezume reviewed Oct 26, 2019

View reviewed changes

refactor(radio-input-group): rename cloned child

320e6a8

feat(radio-input): modify wrapper to support a controlled tooltip

e3b73af

tdeekens reviewed Oct 29, 2019

View reviewed changes

dogayuksel requested a review from montezume October 30, 2019 09:18

dogayuksel added 👨‍👩‍👦‍👦 Status: Dev Review and removed 🚧 Status: WIP Work in progress labels Oct 30, 2019

montezume approved these changes Oct 30, 2019

View reviewed changes

dogayuksel added the 🚀 Status: ship it label Oct 30, 2019

Merge master into dy-components-prop-for-radio-input-option

70c827e

tdeekens approved these changes Oct 30, 2019

View reviewed changes

tdeekens reviewed Oct 30, 2019

View reviewed changes

montezume merged commit a939a97 into master Oct 30, 2019

montezume deleted the dy-components-prop-for-radio-input-option branch October 30, 2019 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat(radio-input): add components prop to radio input option #1139

[WIP] feat(radio-input): add components prop to radio input option #1139

dogayuksel commented Oct 25, 2019

dogayuksel commented Oct 25, 2019

tdeekens left a comment

tdeekens Oct 25, 2019

montezume Oct 25, 2019

tdeekens Oct 25, 2019 •

edited

Loading

montezume Oct 28, 2019

dogayuksel Oct 28, 2019

tdeekens Oct 28, 2019

montezume Oct 28, 2019

montezume Oct 26, 2019

dogayuksel Oct 28, 2019

dogayuksel commented Oct 28, 2019

tdeekens commented Oct 28, 2019

tdeekens Oct 29, 2019

dogayuksel Oct 29, 2019

dogayuksel Oct 29, 2019

tdeekens Oct 29, 2019

montezume left a comment

montezume commented Oct 30, 2019

montezume commented Oct 30, 2019

tdeekens Oct 30, 2019

[WIP] feat(radio-input): add components prop to radio input option #1139

[WIP] feat(radio-input): add components prop to radio input option #1139

Conversation

dogayuksel commented Oct 25, 2019

Summary

dogayuksel commented Oct 25, 2019

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dogayuksel commented Oct 28, 2019

tdeekens commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume left a comment

Choose a reason for hiding this comment

montezume commented Oct 30, 2019

montezume commented Oct 30, 2019

Choose a reason for hiding this comment

tdeekens Oct 25, 2019 •

edited

Loading