fix(factories): reverted changed for not passing props to elements in the factories #759
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
| { value: <div {...{ overridden: true } as any} /> }, | ||
| ) | ||
| itOverridesDefaultPropsWithFalseyProps('element', { | ||
| value: <div {...{ undef: undefined, nil: null, zero: 0, empty: '' } as any} />, |
There was a problem hiding this comment.
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:
| value: <div {...{ undef: undefined, nil: null, zero: 0, empty: '' } as any} />, | |
| value: <div undef={undefined} nil={null} zero={0} empty='' />, |
There was a problem hiding this comment.
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.
docs/src/views/ShorthandProps.tsx
Outdated
| 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, you may end up with invalid props applied on HTML |
There was a problem hiding this comment.
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.
The reason I would suggest to rephrase original one is elimination of invalid props - in fact, there is nothing invalid in props provided, it is just about element may be not aware of necessity to handle them.
There was a problem hiding this comment.
Speaking of this, maybe using the <i /> is not the best example? Should we consider using another Stardust component, like for example?
This PR is reverting some of the changes introduced in #496 regarding not passing props in the factories if the element is a React component, in order to fix #671