Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(factories): reverted changed for not passing props to elements in the factories #759

Merged
merged 9 commits into from
Jan 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix teams theme `Status` and `Chat.Message` styles ([#747](https://github.com/stardust-ui/react/pull/747))
- Fix `Popup` - do not stop event propagation when pressing Esc on trigger element @sophieH29 ([#750](https://github.com/stardust-ui/react/pull/750))
- Fix alignment of `Layout`'s `main` area @kuzhelov ([#752](https://github.com/stardust-ui/react/pull/752))
- Forwarding props for `createShorthand` calls if the value is a React element @mnajdova ([#759](https://github.com/stardust-ui/react/pull/759))

### Features
- Rename `Slot` component to `Box` and export it @Bugaa92 ([#713](https://github.com/stardust-ui/react/pull/713))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const items = [
key: 'message-id-5',
},
{
message: <Divider content="Today" color="primary" important />,
children: <Divider content="Today" color="primary" important />,
key: 'message-id-6',
},
{
Expand Down
12 changes: 11 additions & 1 deletion docs/src/prototypes/chatPane/index.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as React from 'react'
import { Provider } from '@stardust-ui/react'
import { Provider, Divider } from '@stardust-ui/react'

import ChatPaneLayout from './chatPaneLayout'
import { getChatMock } from './services'
import { pxToRem } from '../../../../src/lib'

const chatMock = getChatMock({ msgCount: 40, userCount: 6 })
const dividerSelector = `& .${Divider.className}`

export default () => (
<Provider
Expand All @@ -14,6 +16,14 @@ export default () => (
start: { display: 'block' },
end: { display: 'block' },
},
ChatItem: {
root: {
[dividerSelector]: {
marginLeft: `-${pxToRem(40)}`,
marginRight: `-${pxToRem(40)}`,
},
},
},
},
}}
>
Expand Down
13 changes: 6 additions & 7 deletions docs/src/views/ShorthandProps.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,13 @@ const ShorthandProps = props => (
<blockquote>
<strong>
There is a very important caveat here, though: whenever React Element is directly used as a
shorthand value, it becomes client's responsibility to handle some aspects that Stardust was
originally responsible for (such as {code('styles')} or {code('accessibility')}).
shorthand value, all props that Stardust has created for the slot's Component will be spread
on the passed element. This means that provided element should be able to handle Stardust
props - while this requirement is satisfied for all Stardust components, you should be aware
of that when either HTML or any third-party elements are provided.
</strong>{' '}
This is because, in contrast to other forms of shorthand values, Stardust-evaluated props
cannot be safely passed to the element which type is, generally, doesn't allow Stardust to
make any prior assumptions about. Due to this limitation, you should strive to use other
options for shorthand values whenever is possible - for instance, this is how previous example
can be rewritten:
Due to this limitation, you should strive to use other options for shorthand values whenever
is possible - for instance, this is how previous example can be rewritten:
</blockquote>
{codeExample([`<Button icon={{ as: 'i', className: 'my-icon' }} />`])}

Expand Down
13 changes: 7 additions & 6 deletions src/lib/factories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,18 +129,16 @@ 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 = valIsPropsObject ? (value as Props) : {}
const usersProps =
(valIsReactElement && (value as React.ReactElement<Props>).props) ||
(valIsPropsObject && (value as Props)) ||
{}

// Override props
let { overrideProps } = options
Expand Down Expand Up @@ -199,6 +197,9 @@ 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 React.createElement(Component, props)

Expand Down
33 changes: 20 additions & 13 deletions test/specs/lib/factories-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,16 @@ 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 = <div {...userProps} />

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(() => ({}))
Expand Down Expand Up @@ -524,21 +534,18 @@ describe('factories', () => {

describe('from an element', () => {
itReturnsAValidElement(<div />)
itAppliesDefaultProps(<div />)
itDoesNotIncludePropsFromMappedProp(<div />)
itMergesClassNames('element', 'user', { value: <div className="user" /> })
itAppliesProps('element', { foo: 'foo' }, { value: <div {...{ foo: 'foo' } as any} /> })

test('forwards original element "as is"', () => {
testCreateShorthand(
{
Component: 'p',
value: (
<span {...{ commonProp: 'originalElement', originalElementProp: true } as any} />
),
defaultProps: { commonProp: 'default', defaultProp: true },
overrideProps: { commonProp: 'override', overrideProp: true },
},
{ commonProp: 'originalElement', originalElementProp: true },
)
itOverridesDefaultProps(
'element',
{ some: 'defaults', overridden: false },
{ some: 'defaults', overridden: true },
{ value: <div {...{ overridden: true } as any} /> },
)
itOverridesDefaultPropsWithFalseyProps('element', {
value: <div {...{ undef: undefined, nil: null, zero: 0, empty: '' } as any} />,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to create these superfluous objects and spread them like this? It would be much simpler to just pass the props, if possible:

Suggested change
value: <div {...{ undef: undefined, nil: null, zero: 0, empty: '' } as any} />,
value: <div undef={undefined} nil={null} zero={0} empty='' />,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript is complaining because these props are not valid for div element, that's why we have this object spread. I can create another object and spread it, but it is much easier indeed to read it if it is inline on the element.

})
})

Expand Down