fix(factories): shorthand fix for applying props to react element (#496)

* addressed PR comments

* addressed all comments

* addressed comments and reimplemented to return element as is

* fixed tests

* fixed examples and removed manual check for `Input` `wrapper` styles

* fixed changelog entry

* addressed comments from layershifter

* addressed Roman's comments

* remove unsupported example

Author: Alexandru Buliga

CHANGELOG.md

@@ -29,6 +29,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Add `https` protocol to all urls used in the scripts and stylesheets in index.ejs @mnajdova ([#571](https://github.com/stardust-ui/react/pull/571))
 - Fix support for fallback values in styles (`color: ['#ccc', 'rgba(0, 0, 0, 0.5)']`) @miroslavstastny ([#573](https://github.com/stardust-ui/react/pull/573))
 - Fix styles for RTL mode of doc site component examples @kuzhelov ([#579](https://github.com/stardust-ui/react/pull/579))
+- Prevent blind props forwarding for `createShorthand` calls if the value is a React element and remove manual check for `Input` `wrapper` @Bugaa92 ([#496](https://github.com/stardust-ui/react/pull/496))
 
 ### Features
 - `Ref` components uses `forwardRef` API by default @layershifter ([#491](https://github.com/stardust-ui/react/pull/491))

docs/src/examples/components/Input/Variations/InputExampleInputSlot.shorthand.tsx

@@ -22,20 +22,6 @@ const InputExampleInputSlot = () => (
         styles: inputStyles,
       }}
     />
-
-    <Text content="Wrapped Input with existing component:" />
-    <Input
-      placeholder="Search..."
-      role="presentation"
-      input={
-        <Text
-          as="input"
-          placeholder="Placeholder Override..."
-          role="checkbox"
-          styles={inputStyles}
-        />
-      }
-    />
   </Grid>
 )
 

docs/src/examples/components/Input/Variations/InputExampleWrapperSlot.shorthand.tsx

@@ -23,22 +23,6 @@ const InputExampleWrapperSlot = () => (
         styles: { padding: '5px', backgroundColor: 'red' },
       }}
     />
-
-    <Text content="Wrapped Input with existing component:" />
-    <Input
-      placeholder="Search..."
-      tabIndex={-1}
-      styles={{ color: 'blue', backgroundColor: 'yellow' }}
-      wrapper={<Text tabIndex={0} styles={{ padding: '5px', backgroundColor: 'red' }} />}
-    />
-
-    <Text content="Wrapped Input with custom element:" />
-    <Input
-      placeholder="Search..."
-      tabIndex={-1}
-      styles={{ color: 'blue', backgroundColor: 'yellow' }}
-      wrapper={<span tabIndex={0} style={{ padding: '5px', backgroundColor: 'red' }} />}
-    />
   </Grid>
 )
 

docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx

@@ -8,8 +8,8 @@ const handleChange = () => {
 const RadioGroupItemExample = () => (
   <RadioGroup
     items={[
-      <RadioGroup.Item key="1" label="Make your choice" value="1" checkedChanged={handleChange} />,
-      <RadioGroup.Item key="2" label="Another option" value="2" checkedChanged={handleChange} />,
+      { key: '1', label: 'Make your choice', value: '1', checkedChanged: handleChange },
+      { key: '2', label: 'Another option', value: '2', checkedChanged: handleChange },
     ]}
   />
 )

docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx

@@ -4,7 +4,10 @@ import { RadioGroup } from '@stardust-ui/react'
 const RadioGroupItemExampleCheckedShorthand = () => (
   <RadioGroup
     defaultCheckedValue="1"
-    items={[<RadioGroup.Item key="1" label="This radio comes pre-checked" value="1" />]}
+    items={[
+      { key: '1', label: 'This radio comes pre-checked', value: '1' },
+      { key: '2', label: 'This radio is not pre-checked', value: '2' },
+    ]}
   />
 )
 

docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleDisabled.shorthand.tsx

@@ -4,8 +4,8 @@ import { RadioGroup } from '@stardust-ui/react'
 const RadioGroupItemExampleDisabledShorthand = () => (
   <RadioGroup
     items={[
-      <RadioGroup.Item key="1" label="Disabled" value="1" disabled />,
-      <RadioGroup.Item key="2" label="Enabled" value="2" />,
+      { key: '1', label: 'Disabled', value: '1', disabled: true },
+      { key: '2', label: 'Enabled', value: '2' },
     ]}
   />
 )

docs/src/examples/components/RadioGroup/Types/RadioGroupColorPickerExample.shorthand.tsx

@@ -2,9 +2,33 @@ import React from 'react'
 import { Divider, RadioGroup } from '@stardust-ui/react'
 
 class RadioGroupColorPickerExample extends React.Component {
-  createIcon = value => {
+  state = { selectedValue: '' }
+
+  render() {
+    const { selectedValue } = this.state
+    return (
+      <div>
+        The selected value is: {selectedValue}
+        <Divider />
+        <RadioGroup
+          defaultCheckedValue="pink"
+          items={['pink', 'blue', 'green', 'red', 'orange'].map(color => ({
+            key: color,
+            value: color,
+            name: color,
+            'aria-label': color,
+            icon: this.createIcon(color),
+          }))}
+          checkedValueChanged={(e, props) => this.setState({ selectedValue: props.value })}
+        />
+      </div>
+    )
+  }
+
+  createIcon(value) {
     const { selectedValue } = this.state
     const isSelected = selectedValue === value
+
     return {
       variables: {
         backgroundColor: value,
@@ -22,37 +46,6 @@ class RadioGroupColorPickerExample extends React.Component {
       },
     }
   }
-
-  items = () => {
-    const colors = ['pink', 'blue', 'green', 'red', 'orange']
-    return colors.map(color => (
-      <RadioGroup.Item
-        value={color}
-        icon={this.createIcon(color)}
-        name={color}
-        key={color}
-        aria-label={color}
-      />
-    ))
-  }
-
-  state = { selectedValue: '' }
-  handleChange = (e, props) => {
-    this.setState({ selectedValue: props.value })
-  }
-  render() {
-    const { selectedValue } = this.state
-    return (
-      <div>
-        The selected value is: {selectedValue}
-        <Divider />
-        <RadioGroup
-          defaultCheckedValue="pink"
-          items={this.items()}
-          checkedValueChanged={this.handleChange}
-        />
-      </div>
-    )
-  }
 }
+
 export default RadioGroupColorPickerExample

src/components/Input/Input.tsx

@@ -146,13 +146,8 @@ class Input extends AutoControlledComponent<Extendable<InputProps>, InputState>
             })}
           </>
         ),
+        styles: styles.root,
         ...rest,
-
-        // do not pass Stardust 'styles' prop
-        // in case if React Element was used to define 'wrapper'
-        ...(!React.isValidElement(wrapper) && {
-          styles: styles.root,
-        }),
       },
       overrideProps: {
         as: (wrapper && (wrapper as any).as) || ElementType,

src/lib/factories.tsx

@@ -126,16 +126,18 @@ function createShorthandFromValue(
     )
   }
 
+  // return value 'as is' if it a ReactElement
+  if (valIsReactElement) {
+    return value as React.ReactElement<Props>
+  }
+
   // ----------------------------------------
   // Build up props
   // ----------------------------------------
   const { defaultProps = {} } = options
 
   // User's props
-  const usersProps =
-    (valIsReactElement && (value as React.ReactElement<Props>).props) ||
-    (valIsPropsObject && (value as Props)) ||
-    {}
+  const usersProps = valIsPropsObject ? (value as Props) : {}
 
   // Override props
   let { overrideProps } = options
@@ -191,9 +193,6 @@ function createShorthandFromValue(
     return render(Component, props)
   }
 
-  // Clone ReactElements
-  if (valIsReactElement) return React.cloneElement(value as React.ReactElement<Props>, props)
-
   // Create ReactElements from built up props
   if (valIsPrimitive || valIsPropsObject) return <Component {...props} />
 

test/specs/commonTests/implementsWrapperProp.tsx

@@ -7,14 +7,14 @@ import { ShorthandValue } from 'utils'
 
 export interface ImplementsWrapperPropOptions {
   wrapppedComponentSelector: any
-  wrappperComponentSelector?: any
+  WrapperComponent?: any
 }
 
 const implementsWrapperProp = <P extends { wrapper: ShorthandValue }>(
   Component: React.ReactType<P>,
   options: ImplementsWrapperPropOptions,
 ) => {
-  const { wrapppedComponentSelector, wrappperComponentSelector = Slot.defaultProps.as } = options
+  const { wrapppedComponentSelector, WrapperComponent = Slot } = options
 
   const wrapperTests = (wrapper: ReactWrapper) => {
     expect(wrapper.length).toBeGreaterThan(0)
@@ -23,11 +23,7 @@ const implementsWrapperProp = <P extends { wrapper: ShorthandValue }>(
 
   describe('"wrapper" prop', () => {
     it('wraps the component by default', () => {
-      wrapperTests(mount(<Component />).find(wrappperComponentSelector))
-    })
-
-    it('wraps the component with a custom element', () => {
-      wrapperTests(mount(<Component wrapper={<span />} />).find('span'))
+      wrapperTests(mount(<Component />).find(WrapperComponent))
     })
 
     it('wraps the component with a custom element using "as" prop', () => {

test/specs/components/RadioGroup/RadioGroup-test.tsx

@@ -60,7 +60,7 @@ describe('RadioGroup', () => {
     })
   })
 
-  const itemsTest = getItems => {
+  const itemsTest = (getItems: Function, isShorthandApiTest: boolean = true) => {
     it('renders children', () => {
       const items = mountWithProvider(<RadioGroup items={getItems()} />).find('RadioGroupItem')
 
@@ -101,31 +101,33 @@ describe('RadioGroup', () => {
       })
     })
 
-    describe('click event handler', () => {
-      it('should set the value when item is clicked', () => {
-        const checkedValueChanged = jest.fn()
-        const wrapper = mountWithProvider(
-          <RadioGroup items={getItems()} checkedValueChanged={checkedValueChanged} />,
-        )
-        const radioGroupItems = wrapper.find('RadioGroupItem')
+    if (isShorthandApiTest) {
+      describe('click event handler', () => {
+        it('should set "checked" when item is clicked', () => {
+          const checkedValueChanged = jest.fn()
+          const wrapper = mountWithProvider(
+            <RadioGroup items={getItems()} checkedValueChanged={checkedValueChanged} />,
+          )
+          const radioGroupItems = wrapper.find('RadioGroupItem')
 
-        radioGroupItems
-          .at(1)
-          .find('div')
-          .first()
-          .simulate('click')
+          radioGroupItems
+            .at(1)
+            .find('div')
+            .first()
+            .simulate('click')
 
-        const updatedItems = wrapper.find('RadioGroupItem')
+          const updatedItems = wrapper.find('RadioGroupItem')
 
-        expect(updatedItems.at(0).props().checked).toBe(false)
-        expect(updatedItems.at(1).props().checked).toBe(true)
+          expect(updatedItems.at(0).props().checked).toBe(false)
+          expect(updatedItems.at(1).props().checked).toBe(true)
 
-        expect(checkedValueChanged).toHaveBeenCalledWith(
-          expect.anything(),
-          expect.objectContaining({ value: 'test-value2' }),
-        )
+          expect(checkedValueChanged).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ value: 'test-value2' }),
+          )
+        })
       })
-    })
+    }
 
     it('should not call checkedValueChanged when index did not change', () => {
       const checkedValueChanged = jest.fn()
@@ -147,21 +149,23 @@ describe('RadioGroup', () => {
       expect(checkedValueChanged).not.toHaveBeenCalled()
     })
 
-    it('should not set the value when disabled item is clicked', () => {
-      const wrapper = mountWithProvider(<RadioGroup items={getItems({ disabledItem: 1 })} />)
-      const radioGroupItems = wrapper.find('RadioGroupItem')
+    if (isShorthandApiTest) {
+      it('should not set "checked" when disabled item is clicked', () => {
+        const wrapper = mountWithProvider(<RadioGroup items={getItems({ disabledItem: 1 })} />)
+        const radioGroupItems = wrapper.find('RadioGroupItem')
 
-      radioGroupItems
-        .at(1)
-        .find('div')
-        .first()
-        .simulate('click')
+        radioGroupItems
+          .at(1)
+          .find('div')
+          .first()
+          .simulate('click')
 
-      const updatedItems = wrapper.find('RadioGroupItem')
+        const updatedItems = wrapper.find('RadioGroupItem')
 
-      expect(updatedItems.at(0).props().checked).toBe(false)
-      expect(updatedItems.at(1).props().checked).toBe(false)
-    })
+        expect(updatedItems.at(0).props().checked).toBe(false)
+        expect(updatedItems.at(1).props().checked).toBe(false)
+      })
+    }
 
     describe('keyDown event handler', () => {
       const testKeyDown = (testName, items, initialValue, keyCode, expectedValue) => {
@@ -244,6 +248,6 @@ describe('RadioGroup', () => {
       })
     }
 
-    itemsTest(getChildrenItems)
+    itemsTest(getChildrenItems, false)
   })
 })