[Labs/Popover2] Ease Popover2 migration with prop shims #1581
Conversation
Add testsPreview: documentation |
packages/labs/test/popover2Tests.tsx
Outdated
| child | ||
| </Popover2>, | ||
| ); | ||
| { |
There was a problem hiding this comment.
what is this trick called? closures?
There was a problem hiding this comment.
Ok wow this is pretty cool! Makes the setup/assertion/setup/assertion pattern very clear.
But might this still be better accomplished with a helper function? Instead of repeating these 5 lines many times.
assertPlacement(wrapper, expected)
There was a problem hiding this comment.
it's called block scoping. you can read more about it here:
https://github.com/getify/You-Dont-Know-JS/blob/e342ec48f9e19c873e9a81dd36013c5875317dcc/es6%20%26%20beyond/ch2.md#block-scoped-declarations
| @@ -0,0 +1,57 @@ | |||
| /* | |||
There was a problem hiding this comment.
I vote to rename this file to popoverMigrationUtils.ts. Suffixing bags of functions with -Utils.ts ensures we know exactly what that file is at a glance, and the consistency is nice too.
| * @param candidates the ordered list of value candidates | ||
| * @param defaultValue the fallback value | ||
| */ | ||
| export function resolvePropValue<T>(candidates: Array<T | undefined>, defaultValue: T): T { |
There was a problem hiding this comment.
How about (defaultValue, ...candidates)? Won't need to clone the array before reversing.
| case Position.LEFT_TOP: | ||
| return "left-start"; | ||
| default: | ||
| return assertNever(position); |
There was a problem hiding this comment.
I'm a fan of one line when it's this simple
case X: return Y;
There was a problem hiding this comment.
I bet prettier is going to have an opinion.
| @@ -0,0 +1,57 @@ | |||
| /* | |||
packages/labs/src/common/errors.ts
Outdated
|
|
||
| export const POPOVER_WARN_DEPRECATED_IS_DISABLED = `${deprec} <Popover2> isDisabled is deprecated. Use disabled.`; | ||
| export const POPOVER_WARN_DEPRECATED_IS_MODAL = `${deprec} <Popover2> isModal is deprecated. Use hasBackdrop.`; | ||
| export const POPOVER_WARN_DEPRECATED_POSITION = `${deprec} <Popover2> position is deprecated. Use placement.`; |
| * Migrate a legacy position into a placement. | ||
| * @param position the position to convert | ||
| */ | ||
| export function migratePosition(position: Position): Placement { |
packages/labs/test/popover2Tests.tsx
Outdated
| child | ||
| </Popover2>, | ||
| ); | ||
| { |
There was a problem hiding this comment.
Ok wow this is pretty cool! Makes the setup/assertion/setup/assertion pattern very clear.
But might this still be better accomplished with a helper function? Instead of repeating these 5 lines many times.
assertPlacement(wrapper, expected)
|
So, I had some second thoughts on this. I don't like how easy it is to bypass the special getters, and access the props directly. Seems like it will really hurt maintainability. So, what if we added an HOC what transformed the props before they were consumed by the component. This would allow us to safely use /**
* React component decorator that transforms incoming props according to the given transformation function.
*
* Example usage:
* @transformProps(props => {
* let someProp = someDefaultValue;
* if (props.someProp !== undefined) {
* someProp = props.someProp;
* } else if (props.someDeprecatedProp !== undefined) {
* someProp = props.someDeprecatedProp;
* }
*
* return {
* ...props,
* someProp,
* };
* })
* class MyComponent extends React.Component<MyComponentProps, {}> {
* public render() {
* // Here, this.props is the transformed version
* // ...
* }
* }
*
* @param transformer a function that converts the input props into the desired output props
* @returns a decorator that wraps the target component in a higher-order stateless functional component which
* transforms the props before they are given to the created element
*/
export function transformProps<P>(transformer: (props: P) => P) {
return (target: any): any => {
return (props: P) => React.createElement(target, transformer(props));
};
}Only issue I can foresee with this is with enzyme. Doesn't sound like it plays well with HOCs since our tests check the state of the |
|
@brieb that sounds promising! I like the |
Address PR feedbackPreview: documentation | table |
|
Getting the tests working with the HOC proved to be quite challenging. We can maybe revisit this later on. Instead, I changed this to use a derived state value. There is some precedence for this elsewhere in the code when we derive internal values for controlled / uncontrolled components. So, it seemed like a less foreign pattern to get the final value off the state than to be sure to use a random getter every time you access the value. So, not quite what I was going for with the decorator, but a step in the right direction. Thoughts? |
Use state for derived migrated prop valuesPreview: documentation | table |
|
@brieb That sounds good to me. |
|
Says "You're not authorized to merge this pull request." |
Fixes #1553
Checklist