Skip to content

Conversation

@lasseborly
Copy link

This PR reflects some of the thought put out in #26 (comment).

From reading some of the recent PR's I can see that having a literal boolean type on Transition has been discussed in #84 but never seemed to have gotten implemented.
The TS compiler throws an error if a literal boolean false type is being substituted for this PR's basic boolean type. A simple type discrepancy. Therefore boolean is used instead.

I'll modify as needed.

Thanks for a great lib @drcmda 👍

@lasseborly lasseborly changed the title Add Transition TS types and prop-types Update Transition render and children type definition Jul 23, 2018
@lasseborly lasseborly changed the title Update Transition render and children type definition Update Transitio type definition Jul 23, 2018
Copy link
Member

@drcmda drcmda left a comment

Choose a reason for hiding this comment

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

The readme additions are great. Could you take another look into the definition though - there seems to be a mistake as children/render can't take boolean types, they can take a ternary which yields a function or undefined.

children: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.func),
PropTypes.func,
PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

@lasseborly {visible && fn} isn't a boolean, though. It either results in a function of undefined.

render: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.func),
PropTypes.func,
PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

same here.

@lasseborly lasseborly changed the title Update Transitio type definition Update Transition type definition Jul 23, 2018
@lasseborly
Copy link
Author

@drcmda I see your point. Will correct.

@lasseborly
Copy link
Author

@drcmda corrected!

{toggle ? ComponentA : ComponentB}
{toggle
? styles => <ComponentA style={styles} />
: styles => <ComponentB style={styles} />

Choose a reason for hiding this comment

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

This will definitely clear up some confusion 👍

@lasseborly
Copy link
Author

@drcmda is there anything else I can do to make this go through?
Would love to get off of my own fork and correct my dependency.

@drcmda
Copy link
Member

drcmda commented Jul 29, 2018

@lasseborly i'll merge and release today. wasn't with a computer over the last week, we made a small trip to rome :-)

@lasseborly
Copy link
Author

Sounds lovely @drcmda !

@drcmda
Copy link
Member

drcmda commented Jul 29, 2018

@lasseborly

Just went through, boolean still seems to be part of children and render

  children?:
    | SpringRendererFunc<S, DS>
    | Array<SpringRendererFunc<S, DS>>
    | boolean

  render?:
    | SpringRendererFunc<S, DS>
    | Array<SpringRendererFunc<S, DS>>
    | boolean

Could i see a snippet of code that depends on your fork? Because it seems strange, children/render shouldn't take booleans, only functions and ternaries that yield functions, or undefined.

@lasseborly
Copy link
Author

@drcmda so when we do the single component transition TS interprets that as children/render being a boolean if toggle is falsy.

I made a codesandbox so you can see the problem for yourself.
This problem get's solved if we add the boolean possibility to the TS type def.
This is also why I made a reference to a previous PR #84 which tried to solve this by adding a literal type. The literal type is not the correct solution though, because TS still interprets the expressions to have the possibility of being a boolean.

@lasseborly
Copy link
Author

I can see the argument for not having the boolean prop-type on the JS side of things, since as you pointed out this evaluates to undefined and not specifically false and therefore only a falsy type not an actual boolean.

@drcmda
Copy link
Member

drcmda commented Jul 29, 2018

Just checking if it's correct - i have yet to dig into TS. If it helps TS knowing undefined i guess it's fine. I'll run some errands and release later this day.

@drcmda drcmda merged commit 7b2f77c into pmndrs:master Jul 29, 2018
szjemljanoj pushed a commit to szjemljanoj/react-spring that referenced this pull request Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants