From d0c960bc03ec3f3e32c422d4ef5715b1a42553ef Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Tue, 4 Dec 2018 16:02:11 +0100 Subject: [PATCH 1/9] addressed comments and reimplemented to return element as is --- .../InputExampleWrapperSlot.shorthand.tsx | 16 ------------- src/lib/factories.tsx | 10 ++++---- .../commonTests/implementsWrapperProp.tsx | 10 ++++---- test/specs/lib/factories-test.tsx | 24 +++++++++++++++---- test/utils/index.ts | 1 + 5 files changed, 32 insertions(+), 29 deletions(-) diff --git a/docs/src/examples/components/Input/Variations/InputExampleWrapperSlot.shorthand.tsx b/docs/src/examples/components/Input/Variations/InputExampleWrapperSlot.shorthand.tsx index d6a65cda35..51d8551e72 100644 --- a/docs/src/examples/components/Input/Variations/InputExampleWrapperSlot.shorthand.tsx +++ b/docs/src/examples/components/Input/Variations/InputExampleWrapperSlot.shorthand.tsx @@ -23,22 +23,6 @@ const InputExampleWrapperSlot = () => ( styles: { padding: '5px', backgroundColor: 'red' }, }} /> - - - } - /> - - - } - /> ) diff --git a/src/lib/factories.tsx b/src/lib/factories.tsx index 2684c19ef0..66dd104d74 100644 --- a/src/lib/factories.tsx +++ b/src/lib/factories.tsx @@ -105,9 +105,14 @@ function createShorthandFromValue( const valIsNoop = _.isNil(value) || typeof value === 'boolean' if (valIsNoop && !options.render) return null + // just return ReactElements + const valIsReactElement = React.isValidElement(value) + if (valIsReactElement) { + return value as React.ReactElement + } + const valIsPrimitive = typeof value === 'string' || typeof value === 'number' const valIsPropsObject = _.isPlainObject(value) - const valIsReactElement = React.isValidElement(value) // unhandled type warning if ( @@ -191,9 +196,6 @@ function createShorthandFromValue( return render(Component, props) } - // Clone ReactElements - if (valIsReactElement) return React.cloneElement(value as React.ReactElement, props) - // Create ReactElements from built up props if (valIsPrimitive || valIsPropsObject) return diff --git a/test/specs/commonTests/implementsWrapperProp.tsx b/test/specs/commonTests/implementsWrapperProp.tsx index 9aaaa34f9a..dbb37567de 100644 --- a/test/specs/commonTests/implementsWrapperProp.tsx +++ b/test/specs/commonTests/implementsWrapperProp.tsx @@ -7,14 +7,14 @@ import { ShorthandValue } from 'utils' export interface ImplementsWrapperPropOptions { wrapppedComponentSelector: any - wrappperComponentSelector?: any + WrapperComponent?: any } const implementsWrapperProp =

( Component: React.ReactType

, 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,11 @@ const implementsWrapperProp =

( describe('"wrapper" prop', () => { it('wraps the component by default', () => { - wrapperTests(mount().find(wrappperComponentSelector)) + wrapperTests(mount().find(WrapperComponent)) }) - it('wraps the component with a custom element', () => { - wrapperTests(mount(} />).find('span')) + it('wraps the component with an element of the same type as the wrapper component', () => { + wrapperTests(mount(} />).find(WrapperComponent)) }) it('wraps the component with a custom element using "as" prop', () => { diff --git a/test/specs/lib/factories-test.tsx b/test/specs/lib/factories-test.tsx index 5b3b22461a..a90fe1eab8 100644 --- a/test/specs/lib/factories-test.tsx +++ b/test/specs/lib/factories-test.tsx @@ -10,7 +10,7 @@ import callable from '../../../src/lib/callable' // Utils // ---------------------------------------- -type GetShorthandArgs = { +type ShorthandConfig = { Component?: React.ReactType defaultProps?: Props mappedProp?: string @@ -31,7 +31,7 @@ const getShorthand = ({ generateKey, value, render, -}: GetShorthandArgs) => +}: ShorthandConfig) => createShorthand(Component, mappedProp, value, { defaultProps, overrideProps, @@ -42,7 +42,7 @@ const getShorthand = ({ const isValuePrimitive = (value: ShorthandValue) => typeof value === 'string' || typeof value === 'number' -const testCreateShorthand = (shorthandArgs: GetShorthandArgs, expectedResult: ObjectOf) => +const testCreateShorthand = (shorthandArgs: ShorthandConfig, expectedResult: ObjectOf) => expect(shallow(getShorthand(shorthandArgs)).props()).toEqual(expectedResult) // ---------------------------------------- @@ -111,7 +111,11 @@ const itMergesClassNames = ( }) } -const itAppliesProps = (propsSource, expectedProps, shorthandConfig) => { +const itAppliesProps = ( + propsSource: string, + expectedProps: Props, + shorthandConfig: ShorthandConfig, +) => { test(`applies props from the ${propsSource} props`, () => { testCreateShorthand(shorthandConfig, expectedProps) }) @@ -537,6 +541,18 @@ describe('factories', () => { itOverridesDefaultPropsWithFalseyProps('element', { value:

, }) + + test('applies only the element props and returns the element', () => { + testCreateShorthand( + { + Component: 'p', + value: , + defaultProps: { common: 'default', default: true }, + overrideProps: { common: 'override', override: true }, + }, + { common: 'user', user: true }, + ) + }) }) describe('from a string', () => { diff --git a/test/utils/index.ts b/test/utils/index.ts index a95c193304..44daa78d9b 100644 --- a/test/utils/index.ts +++ b/test/utils/index.ts @@ -1,5 +1,6 @@ export * from './assertNodeContains' export { default as consoleUtil } from './consoleUtil' +export { default as createMockComponent } from './createMockComponent' export { default as domEvent } from './domEvent' export { default as syntheticEvent } from './syntheticEvent' export { default as nextFrame } from './nextFrame' From e6cbc7a2cbacc4f07a453652b606f0dd9c035a86 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Thu, 22 Nov 2018 18:38:56 +0100 Subject: [PATCH 2/9] addressed PR comments --- src/lib/index.ts | 1 + src/lib/reactComponentUtils.ts | 21 +++++++ test/specs/lib/reactComponentUtils-test.tsx | 65 +++++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 src/lib/reactComponentUtils.ts create mode 100644 test/specs/lib/reactComponentUtils-test.tsx diff --git a/src/lib/index.ts b/src/lib/index.ts index 8616a696db..9a7cd3d6f9 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -27,6 +27,7 @@ export { } from './htmlPropsUtils' export { default as isBrowser } from './isBrowser' +export { getComponentName, areTypeNamesEqual } from './reactComponentUtils' export { default as typescriptUtils } from './typescriptUtils' export { default as doesNodeContainClick } from './doesNodeContainClick' export { default as leven } from './leven' diff --git a/src/lib/reactComponentUtils.ts b/src/lib/reactComponentUtils.ts new file mode 100644 index 0000000000..9288373dd7 --- /dev/null +++ b/src/lib/reactComponentUtils.ts @@ -0,0 +1,21 @@ +export const getComponentName = (component: React.ReactType) => { + switch (typeof component) { + case 'function': + return component.displayName || component.name || null // unknown + case 'string': + return component || null // empty string + default: + return null // unknown + } +} + +export const areTypeNamesEqual = (first: React.ReactType, second: React.ReactType): boolean => { + const componentNameOfElement = getComponentName(first) + const componentName = getComponentName(second) + + return ( + componentNameOfElement !== null && + componentName !== null && + componentNameOfElement === componentName + ) +} diff --git a/test/specs/lib/reactComponentUtils-test.tsx b/test/specs/lib/reactComponentUtils-test.tsx new file mode 100644 index 0000000000..0b1e2100e6 --- /dev/null +++ b/test/specs/lib/reactComponentUtils-test.tsx @@ -0,0 +1,65 @@ +import * as React from 'react' +import { getComponentName, areTypeNamesEqual } from 'src/lib/reactComponentUtils' +import { createMockComponent } from 'test/utils' + +describe('getComponentName', () => { + it('returns null for invalid arguments', () => { + [undefined, null, '', {}].map(invalidArg => + expect(getComponentName(invalidArg as any)).toBeFalsy(), + ) + }) + + it('returns the string component itself', () => { + ['div', 'span', 'MyComponent'].map(stringArg => + expect(getComponentName(stringArg)).toEqual(stringArg), + ) + }) + + it('returns the function name', () => { + const MyComponent = createMockComponent('MyComponent') + + function fooComponent() { + return null + } + + expect(getComponentName(fooComponent)).toEqual('fooComponent') + expect(getComponentName(MyComponent)).toEqual('MyComponent') + }) +}) + +describe('areTypeNamesEqual', () => { + it('returns false for invalid arguments', () => { + expect(areTypeNamesEqual(null, undefined)).toBeFalsy() + expect(areTypeNamesEqual(undefined, '')).toBeFalsy() + expect(areTypeNamesEqual('', '')).toBeFalsy() + }) + + describe('for primitive components', () => { + it('returns false for different types', () => { + expect(areTypeNamesEqual(React.createElement('p').type, 'div')).toBeFalsy() + expect(areTypeNamesEqual(React.createElement('div').type, 'span')).toBeFalsy() + }) + + it('returns true for the same type', () => { + expect(areTypeNamesEqual(React.createElement('div').type, 'div')).toBeTruthy() + expect(areTypeNamesEqual(React.createElement('span').type, 'span')).toBeTruthy() + }) + }) + + describe('for functional components', () => { + const MyComponent = createMockComponent('MyComponent') + const OtherComponent = createMockComponent('OtherComponent') + + it('returns false for different types', () => { + expect(areTypeNamesEqual(React.createElement(MyComponent).type, OtherComponent)).toBeFalsy() + expect(areTypeNamesEqual(React.createElement(MyComponent).type, 'OtherComponent')).toBeFalsy() + }) + + it('returns true for the same type', () => { + expect(areTypeNamesEqual(React.createElement(MyComponent).type, MyComponent)).toBeTruthy() + expect( + areTypeNamesEqual(React.createElement(OtherComponent).type, 'OtherComponent'), + ).toBeTruthy() + }) + }) +}) From 0e0d7d48bdc46fb7254aeccf765cb1eedc0c5189 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Fri, 23 Nov 2018 15:58:38 +0100 Subject: [PATCH 3/9] addressed all comments --- src/lib/factories.tsx | 16 +---- src/lib/index.ts | 1 - src/lib/reactComponentUtils.ts | 21 ------- test/specs/lib/factories-test.tsx | 2 +- test/specs/lib/reactComponentUtils-test.tsx | 65 --------------------- test/utils/index.ts | 1 - 6 files changed, 4 insertions(+), 102 deletions(-) delete mode 100644 src/lib/reactComponentUtils.ts delete mode 100644 test/specs/lib/reactComponentUtils-test.tsx diff --git a/src/lib/factories.tsx b/src/lib/factories.tsx index 66dd104d74..d8be6cf458 100644 --- a/src/lib/factories.tsx +++ b/src/lib/factories.tsx @@ -106,8 +106,7 @@ function createShorthandFromValue( if (valIsNoop && !options.render) return null // just return ReactElements - const valIsReactElement = React.isValidElement(value) - if (valIsReactElement) { + if (React.isValidElement(value)) { return value as React.ReactElement } @@ -115,13 +114,7 @@ function createShorthandFromValue( const valIsPropsObject = _.isPlainObject(value) // unhandled type warning - if ( - process.env.NODE_ENV !== 'production' && - !valIsPrimitive && - !valIsPropsObject && - !valIsReactElement && - !valIsNoop - ) { + if (process.env.NODE_ENV !== 'production' && !valIsPrimitive && !valIsPropsObject && !valIsNoop) { console.error( [ 'Shorthand value must be a string|number|object|ReactElements.', @@ -137,10 +130,7 @@ function createShorthandFromValue( const { defaultProps = {} } = options // User's props - const usersProps = - (valIsReactElement && (value as React.ReactElement).props) || - (valIsPropsObject && (value as Props)) || - {} + const usersProps = valIsPropsObject ? (value as Props) : {} // Override props let { overrideProps } = options diff --git a/src/lib/index.ts b/src/lib/index.ts index 9a7cd3d6f9..8616a696db 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -27,7 +27,6 @@ export { } from './htmlPropsUtils' export { default as isBrowser } from './isBrowser' -export { getComponentName, areTypeNamesEqual } from './reactComponentUtils' export { default as typescriptUtils } from './typescriptUtils' export { default as doesNodeContainClick } from './doesNodeContainClick' export { default as leven } from './leven' diff --git a/src/lib/reactComponentUtils.ts b/src/lib/reactComponentUtils.ts deleted file mode 100644 index 9288373dd7..0000000000 --- a/src/lib/reactComponentUtils.ts +++ /dev/null @@ -1,21 +0,0 @@ -export const getComponentName = (component: React.ReactType) => { - switch (typeof component) { - case 'function': - return component.displayName || component.name || null // unknown - case 'string': - return component || null // empty string - default: - return null // unknown - } -} - -export const areTypeNamesEqual = (first: React.ReactType, second: React.ReactType): boolean => { - const componentNameOfElement = getComponentName(first) - const componentName = getComponentName(second) - - return ( - componentNameOfElement !== null && - componentName !== null && - componentNameOfElement === componentName - ) -} diff --git a/test/specs/lib/factories-test.tsx b/test/specs/lib/factories-test.tsx index a90fe1eab8..e8ebe056d4 100644 --- a/test/specs/lib/factories-test.tsx +++ b/test/specs/lib/factories-test.tsx @@ -542,7 +542,7 @@ describe('factories', () => { value:
, }) - test('applies only the element props and returns the element', () => { + test('forwards original element "as is"', () => { testCreateShorthand( { Component: 'p', diff --git a/test/specs/lib/reactComponentUtils-test.tsx b/test/specs/lib/reactComponentUtils-test.tsx deleted file mode 100644 index 0b1e2100e6..0000000000 --- a/test/specs/lib/reactComponentUtils-test.tsx +++ /dev/null @@ -1,65 +0,0 @@ -import * as React from 'react' -import { getComponentName, areTypeNamesEqual } from 'src/lib/reactComponentUtils' -import { createMockComponent } from 'test/utils' - -describe('getComponentName', () => { - it('returns null for invalid arguments', () => { - [undefined, null, '', {}].map(invalidArg => - expect(getComponentName(invalidArg as any)).toBeFalsy(), - ) - }) - - it('returns the string component itself', () => { - ['div', 'span', 'MyComponent'].map(stringArg => - expect(getComponentName(stringArg)).toEqual(stringArg), - ) - }) - - it('returns the function name', () => { - const MyComponent = createMockComponent('MyComponent') - - function fooComponent() { - return null - } - - expect(getComponentName(fooComponent)).toEqual('fooComponent') - expect(getComponentName(MyComponent)).toEqual('MyComponent') - }) -}) - -describe('areTypeNamesEqual', () => { - it('returns false for invalid arguments', () => { - expect(areTypeNamesEqual(null, undefined)).toBeFalsy() - expect(areTypeNamesEqual(undefined, '')).toBeFalsy() - expect(areTypeNamesEqual('', '')).toBeFalsy() - }) - - describe('for primitive components', () => { - it('returns false for different types', () => { - expect(areTypeNamesEqual(React.createElement('p').type, 'div')).toBeFalsy() - expect(areTypeNamesEqual(React.createElement('div').type, 'span')).toBeFalsy() - }) - - it('returns true for the same type', () => { - expect(areTypeNamesEqual(React.createElement('div').type, 'div')).toBeTruthy() - expect(areTypeNamesEqual(React.createElement('span').type, 'span')).toBeTruthy() - }) - }) - - describe('for functional components', () => { - const MyComponent = createMockComponent('MyComponent') - const OtherComponent = createMockComponent('OtherComponent') - - it('returns false for different types', () => { - expect(areTypeNamesEqual(React.createElement(MyComponent).type, OtherComponent)).toBeFalsy() - expect(areTypeNamesEqual(React.createElement(MyComponent).type, 'OtherComponent')).toBeFalsy() - }) - - it('returns true for the same type', () => { - expect(areTypeNamesEqual(React.createElement(MyComponent).type, MyComponent)).toBeTruthy() - expect( - areTypeNamesEqual(React.createElement(OtherComponent).type, 'OtherComponent'), - ).toBeTruthy() - }) - }) -}) diff --git a/test/utils/index.ts b/test/utils/index.ts index 44daa78d9b..a95c193304 100644 --- a/test/utils/index.ts +++ b/test/utils/index.ts @@ -1,6 +1,5 @@ export * from './assertNodeContains' export { default as consoleUtil } from './consoleUtil' -export { default as createMockComponent } from './createMockComponent' export { default as domEvent } from './domEvent' export { default as syntheticEvent } from './syntheticEvent' export { default as nextFrame } from './nextFrame' From ac922b33174223968939a1b78244019a14ea07d3 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Tue, 4 Dec 2018 16:48:46 +0100 Subject: [PATCH 4/9] fixed tests --- src/lib/factories.tsx | 2 +- .../commonTests/implementsWrapperProp.tsx | 4 - .../components/RadioGroup/RadioGroup-test.tsx | 76 ++++++++++--------- test/specs/lib/factories-test.tsx | 21 ----- 4 files changed, 43 insertions(+), 60 deletions(-) diff --git a/src/lib/factories.tsx b/src/lib/factories.tsx index d8be6cf458..6f5cffb085 100644 --- a/src/lib/factories.tsx +++ b/src/lib/factories.tsx @@ -105,7 +105,7 @@ function createShorthandFromValue( const valIsNoop = _.isNil(value) || typeof value === 'boolean' if (valIsNoop && !options.render) return null - // just return ReactElements + // return value as is if it a ReactElement if (React.isValidElement(value)) { return value as React.ReactElement } diff --git a/test/specs/commonTests/implementsWrapperProp.tsx b/test/specs/commonTests/implementsWrapperProp.tsx index dbb37567de..a2feb9a964 100644 --- a/test/specs/commonTests/implementsWrapperProp.tsx +++ b/test/specs/commonTests/implementsWrapperProp.tsx @@ -26,10 +26,6 @@ const implementsWrapperProp =

( wrapperTests(mount().find(WrapperComponent)) }) - it('wraps the component with an element of the same type as the wrapper component', () => { - wrapperTests(mount(} />).find(WrapperComponent)) - }) - it('wraps the component with a custom element using "as" prop', () => { wrapperTests(mount().find('p')) }) diff --git a/test/specs/components/RadioGroup/RadioGroup-test.tsx b/test/specs/components/RadioGroup/RadioGroup-test.tsx index b875c59326..7384031238 100644 --- a/test/specs/components/RadioGroup/RadioGroup-test.tsx +++ b/test/specs/components/RadioGroup/RadioGroup-test.tsx @@ -60,7 +60,13 @@ describe('RadioGroup', () => { }) }) - const itemsTest = getItems => { + const itemsTest = ({ + getItems, + isChildrenApiTest = false, + }: { + getItems: Function + isChildrenApiTest?: boolean + }) => { it('renders children', () => { const items = mountWithProvider().find('RadioGroupItem') @@ -102,29 +108,30 @@ describe('RadioGroup', () => { }) describe('click event handler', () => { - it('should set the value when item is clicked', () => { - const checkedValueChanged = jest.fn() - const wrapper = mountWithProvider( - , - ) - const radioGroupItems = wrapper.find('RadioGroupItem') + !isChildrenApiTest && + it('should set the value when item is clicked', () => { + const checkedValueChanged = jest.fn() + const wrapper = mountWithProvider( + , + ) + 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', () => { @@ -147,21 +154,22 @@ describe('RadioGroup', () => { expect(checkedValueChanged).not.toHaveBeenCalled() }) - it('should not set the value when disabled item is clicked', () => { - const wrapper = mountWithProvider() - const radioGroupItems = wrapper.find('RadioGroupItem') + !isChildrenApiTest && + it('should not set the value when disabled item is clicked', () => { + const wrapper = mountWithProvider() + 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) => { @@ -234,7 +242,7 @@ describe('RadioGroup', () => { } describe('shorthand API for items', () => { - itemsTest(getShorthandItems) + itemsTest({ getItems: getShorthandItems }) }) describe('children API for items', () => { @@ -244,6 +252,6 @@ describe('RadioGroup', () => { }) } - itemsTest(getChildrenItems) + itemsTest({ getItems: getChildrenItems, isChildrenApiTest: true }) }) }) diff --git a/test/specs/lib/factories-test.tsx b/test/specs/lib/factories-test.tsx index e8ebe056d4..2d5351c761 100644 --- a/test/specs/lib/factories-test.tsx +++ b/test/specs/lib/factories-test.tsx @@ -486,16 +486,6 @@ describe('factories', () => { testCreateShorthand({ overrideProps, value: testValue }, overrideProps()) }) - test("is called with the user's element's and default props", () => { - const defaultProps = { 'data-some': 'defaults' } - const overrideProps = jest.fn(() => ({})) - const userProps = { 'data-user': 'props' } - const value =

- - shallow(getShorthand({ defaultProps, overrideProps, value })) - expect(overrideProps).toHaveBeenCalledWith({ ...defaultProps, ...userProps }) - }) - test("is called with the user's props object", () => { const defaultProps = { 'data-some': 'defaults' } const overrideProps = jest.fn(() => ({})) @@ -528,19 +518,8 @@ describe('factories', () => { describe('from an element', () => { itReturnsAValidElement(
) - itAppliesDefaultProps(
) itDoesNotIncludePropsFromMappedProp(
) - itMergesClassNames('element', 'user', { value:
}) itAppliesProps('element', { foo: 'foo' }, { value:
}) - itOverridesDefaultProps( - 'element', - { some: 'defaults', overridden: false }, - { some: 'defaults', overridden: true }, - { value:
}, - ) - itOverridesDefaultPropsWithFalseyProps('element', { - value:
, - }) test('forwards original element "as is"', () => { testCreateShorthand( From 3e9d5662b7dbd4c6b170f87a32478df732710309 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Tue, 4 Dec 2018 17:26:37 +0100 Subject: [PATCH 5/9] fixed examples and removed manual check for `Input` `wrapper` styles --- CHANGELOG.md | 1 + .../Item/RadioGroupItemExample.shorthand.tsx | 4 +- ...RadioGroupItemExampleChecked.shorthand.tsx | 2 +- ...adioGroupItemExampleDisabled.shorthand.tsx | 4 +- ...RadioGroupColorPickerExample.shorthand.tsx | 59 ++++++++----------- src/components/Input/Input.tsx | 7 +-- 6 files changed, 33 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e76b01782..d1b623022d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix truncate styles in Teams team for the `Button`'s `content` prop used as element @mnajdova ([#551](https://github.com/stardust-ui/react/pull/551)) - Fix HTML preview in the editor @layershifter ([#555](https://github.com/stardust-ui/react/pull/555)) - Fix icon overlapping for `iconOnly` prop in `Menu` component with @Bugaa92 ([#486](https://github.com/stardust-ui/react/pull/486)) +- 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 - Add `render` callback as an option for shorthand value @kuzhelov ([#519](https://github.com/stardust-ui/react/pull/519)) diff --git a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx index 25ab8310f9..1f4cb4ee3c 100644 --- a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx +++ b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx @@ -8,8 +8,8 @@ const handleChange = () => { const RadioGroupItemExample = () => ( , - , + { key: '1', label: 'Make your choice', value: '1', checkedChanged: handleChange }, + { key: '2', label: 'Another option', value: '2', checkedChanged: handleChange }, ]} /> ) diff --git a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx index 01c1f5c349..effbef666a 100644 --- a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx +++ b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx @@ -4,7 +4,7 @@ import { RadioGroup } from '@stardust-ui/react' const RadioGroupItemExampleCheckedShorthand = () => ( ]} + items={[{ key: '1', label: 'This radio comes pre-checked', value: '1' }]} /> ) diff --git a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleDisabled.shorthand.tsx b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleDisabled.shorthand.tsx index ba579aa7a7..135f9d4621 100644 --- a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleDisabled.shorthand.tsx +++ b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleDisabled.shorthand.tsx @@ -4,8 +4,8 @@ import { RadioGroup } from '@stardust-ui/react' const RadioGroupItemExampleDisabledShorthand = () => ( , - , + { key: '1', label: 'Disabled', value: '1', disabled: true }, + { key: '2', label: 'Enabled', value: '2' }, ]} /> ) diff --git a/docs/src/examples/components/RadioGroup/Types/RadioGroupColorPickerExample.shorthand.tsx b/docs/src/examples/components/RadioGroup/Types/RadioGroupColorPickerExample.shorthand.tsx index a2a6b936d0..29132a0237 100644 --- a/docs/src/examples/components/RadioGroup/Types/RadioGroupColorPickerExample.shorthand.tsx +++ b/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 ( +
+ The selected value is: {selectedValue} + + ({ + key: color, + value: color, + name: color, + 'aria-label': color, + icon: this.createIcon(color), + }))} + checkedValueChanged={(e, props) => this.setState({ selectedValue: props.value })} + /> +
+ ) + } + + 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 => ( - - )) - } - - state = { selectedValue: '' } - handleChange = (e, props) => { - this.setState({ selectedValue: props.value }) - } - render() { - const { selectedValue } = this.state - return ( -
- The selected value is: {selectedValue} - - -
- ) - } } + export default RadioGroupColorPickerExample diff --git a/src/components/Input/Input.tsx b/src/components/Input/Input.tsx index b403548f95..c9f9898b66 100644 --- a/src/components/Input/Input.tsx +++ b/src/components/Input/Input.tsx @@ -146,13 +146,8 @@ class Input extends AutoControlledComponent, 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, From 6c677035e79334b1b471637ba8768df1578dcf64 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Thu, 6 Dec 2018 12:15:22 +0100 Subject: [PATCH 6/9] fixed changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1b623022d..993f743c4f 100644 --- a/CHANGELOG.md +++ b/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)) @@ -67,7 +68,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix truncate styles in Teams team for the `Button`'s `content` prop used as element @mnajdova ([#551](https://github.com/stardust-ui/react/pull/551)) - Fix HTML preview in the editor @layershifter ([#555](https://github.com/stardust-ui/react/pull/555)) - Fix icon overlapping for `iconOnly` prop in `Menu` component with @Bugaa92 ([#486](https://github.com/stardust-ui/react/pull/486)) -- 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 - Add `render` callback as an option for shorthand value @kuzhelov ([#519](https://github.com/stardust-ui/react/pull/519)) From 40e08f74b5666505c41051fd3824982ab86608d1 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Thu, 6 Dec 2018 18:06:21 +0100 Subject: [PATCH 7/9] addressed comments from layershifter --- ...RadioGroupItemExampleChecked.shorthand.tsx | 5 ++++- .../components/RadioGroup/RadioGroup-test.tsx | 22 ++++++++----------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx index effbef666a..d00e09bb50 100644 --- a/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx +++ b/docs/src/examples/components/RadioGroup/Item/RadioGroupItemExampleChecked.shorthand.tsx @@ -4,7 +4,10 @@ import { RadioGroup } from '@stardust-ui/react' const RadioGroupItemExampleCheckedShorthand = () => ( ) diff --git a/test/specs/components/RadioGroup/RadioGroup-test.tsx b/test/specs/components/RadioGroup/RadioGroup-test.tsx index 7384031238..97834d7889 100644 --- a/test/specs/components/RadioGroup/RadioGroup-test.tsx +++ b/test/specs/components/RadioGroup/RadioGroup-test.tsx @@ -60,13 +60,7 @@ describe('RadioGroup', () => { }) }) - const itemsTest = ({ - getItems, - isChildrenApiTest = false, - }: { - getItems: Function - isChildrenApiTest?: boolean - }) => { + const itemsTest = (getItems: Function, isShorthandApiTest: boolean = true) => { it('renders children', () => { const items = mountWithProvider().find('RadioGroupItem') @@ -107,8 +101,8 @@ describe('RadioGroup', () => { }) }) - describe('click event handler', () => { - !isChildrenApiTest && + if (isShorthandApiTest) { + describe('click event handler', () => { it('should set the value when item is clicked', () => { const checkedValueChanged = jest.fn() const wrapper = mountWithProvider( @@ -132,7 +126,8 @@ describe('RadioGroup', () => { expect.objectContaining({ value: 'test-value2' }), ) }) - }) + }) + } it('should not call checkedValueChanged when index did not change', () => { const checkedValueChanged = jest.fn() @@ -154,7 +149,7 @@ describe('RadioGroup', () => { expect(checkedValueChanged).not.toHaveBeenCalled() }) - !isChildrenApiTest && + if (isShorthandApiTest) { it('should not set the value when disabled item is clicked', () => { const wrapper = mountWithProvider() const radioGroupItems = wrapper.find('RadioGroupItem') @@ -170,6 +165,7 @@ describe('RadioGroup', () => { 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) => { @@ -242,7 +238,7 @@ describe('RadioGroup', () => { } describe('shorthand API for items', () => { - itemsTest({ getItems: getShorthandItems }) + itemsTest(getShorthandItems) }) describe('children API for items', () => { @@ -252,6 +248,6 @@ describe('RadioGroup', () => { }) } - itemsTest({ getItems: getChildrenItems, isChildrenApiTest: true }) + itemsTest(getChildrenItems, false) }) }) From e76bf823da4d44a7106ecdbb5b90d26ccc2fbb6f Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Mon, 10 Dec 2018 12:26:36 +0100 Subject: [PATCH 8/9] addressed Roman's comments --- src/lib/factories.tsx | 19 +++++++++++++------ .../components/RadioGroup/RadioGroup-test.tsx | 4 ++-- test/specs/lib/factories-test.tsx | 10 ++++++---- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/lib/factories.tsx b/src/lib/factories.tsx index 6f5cffb085..c7b18e6552 100644 --- a/src/lib/factories.tsx +++ b/src/lib/factories.tsx @@ -105,16 +105,18 @@ function createShorthandFromValue( const valIsNoop = _.isNil(value) || typeof value === 'boolean' if (valIsNoop && !options.render) return null - // return value as is if it a ReactElement - if (React.isValidElement(value)) { - return value as React.ReactElement - } - const valIsPrimitive = typeof value === 'string' || typeof value === 'number' const valIsPropsObject = _.isPlainObject(value) + const valIsReactElement = React.isValidElement(value) // unhandled type warning - if (process.env.NODE_ENV !== 'production' && !valIsPrimitive && !valIsPropsObject && !valIsNoop) { + if ( + process.env.NODE_ENV !== 'production' && + !valIsPrimitive && + !valIsPropsObject && + !valIsReactElement && + !valIsNoop + ) { console.error( [ 'Shorthand value must be a string|number|object|ReactElements.', @@ -124,6 +126,11 @@ function createShorthandFromValue( ) } + // return value 'as is' if it a ReactElement + if (valIsReactElement) { + return value as React.ReactElement + } + // ---------------------------------------- // Build up props // ---------------------------------------- diff --git a/test/specs/components/RadioGroup/RadioGroup-test.tsx b/test/specs/components/RadioGroup/RadioGroup-test.tsx index 97834d7889..bfebdd896d 100644 --- a/test/specs/components/RadioGroup/RadioGroup-test.tsx +++ b/test/specs/components/RadioGroup/RadioGroup-test.tsx @@ -103,7 +103,7 @@ describe('RadioGroup', () => { if (isShorthandApiTest) { describe('click event handler', () => { - it('should set the value when item is clicked', () => { + it('should set "checked" when item is clicked', () => { const checkedValueChanged = jest.fn() const wrapper = mountWithProvider( , @@ -150,7 +150,7 @@ describe('RadioGroup', () => { }) if (isShorthandApiTest) { - it('should not set the value when disabled item is clicked', () => { + it('should not set "checked" when disabled item is clicked', () => { const wrapper = mountWithProvider() const radioGroupItems = wrapper.find('RadioGroupItem') diff --git a/test/specs/lib/factories-test.tsx b/test/specs/lib/factories-test.tsx index 2d5351c761..279bce263c 100644 --- a/test/specs/lib/factories-test.tsx +++ b/test/specs/lib/factories-test.tsx @@ -525,11 +525,13 @@ describe('factories', () => { testCreateShorthand( { Component: 'p', - value: , - defaultProps: { common: 'default', default: true }, - overrideProps: { common: 'override', override: true }, + value: ( + + ), + defaultProps: { commonProp: 'default', defaultProp: true }, + overrideProps: { commonProp: 'override', overrideProp: true }, }, - { common: 'user', user: true }, + { commonProp: 'originalElement', originalElementProp: true }, ) }) }) From 49c40eca10111ed1f6670b9553d2db69db8387e8 Mon Sep 17 00:00:00 2001 From: Alexandru Buliga Date: Mon, 10 Dec 2018 12:45:36 +0100 Subject: [PATCH 9/9] remove unsupported example --- .../Variations/InputExampleInputSlot.shorthand.tsx | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/docs/src/examples/components/Input/Variations/InputExampleInputSlot.shorthand.tsx b/docs/src/examples/components/Input/Variations/InputExampleInputSlot.shorthand.tsx index 50589dbe28..fb461ab694 100644 --- a/docs/src/examples/components/Input/Variations/InputExampleInputSlot.shorthand.tsx +++ b/docs/src/examples/components/Input/Variations/InputExampleInputSlot.shorthand.tsx @@ -22,20 +22,6 @@ const InputExampleInputSlot = () => ( styles: inputStyles, }} /> - - - - } - /> )