Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Fix falsey default args handling #13911

Closed
wants to merge 2 commits into from
Closed

Conversation

tmeasday
Copy link
Member

Issue: #12767

What I did

Check if default value is undefined, rather than falsey before excluding it.

How to test

  • Is this testable with Jest or Chromatic screenshots?

Yes, see test

  • Does this need a new example in the kitchen sink apps?

I think probably not. I don't think these things are picked up by chromatic anyway so don't end up being very useful to have edge cases, unless they are hard to reproduce.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

💯

@shilman shilman changed the title Allow falsey default args Core: Fix falsey default args handling Feb 15, 2021
@shilman shilman added this to the 6.2 controls milestone Feb 15, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@tmeasday can you take a look at the chromatic changes? probably ok but i'd like a second opinion

@tmeasday
Copy link
Member Author

@shilman -- I approved the first two changes -- this example was a component with a children prop with default value null. I think given current semantics it is correct that that prop should get passed to the component with null value [1]

I'm not sure about the other 8 changes -- are they present on next?

[1] Arguably we shouldn't be setting args to the default value from the component? That might be a discussion for another time.

@shilman
Copy link
Member

shilman commented Feb 16, 2021

@tmeasday I accepted a few more changes (zoom changes are test flake, parameters change is from another PR that slipped in). The remaining ones are all due to this PR I think.

@tmeasday
Copy link
Member Author

tmeasday commented Feb 17, 2021

@shilman OK those stories highlighted a second bug that was obscured by this bug, I think the examples I added in the other PR (#13919) would have made that more obvious.

@tmeasday
Copy link
Member Author

@shilman I am going to merge this into #13919

There's too many overlapping concerns, it's silly to resolve them separately

@shilman shilman closed this Feb 18, 2021
@tmeasday tmeasday deleted the 12767-falsy-default-values branch February 19, 2021 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants