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

Fix layout context prop resolving for boolean types (#567) #568

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atmelmicro
Copy link
Collaborator

Allows boolean props to propagate when layout context is false

Closes #567

@adamkudrna adamkudrna requested a review from mbohal October 7, 2024 09:26
Copy link
Member

Choose a reason for hiding this comment

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

Does it make any sense to have a test for it? Possibly with an example similar to that in the original issue #567.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Ill leave it up to @atmelmicro to decide.

It is one simple condition, so I think that maybe just a comment would work:

// We need to test:
//  * `false` - for when the `contextValue` is boolean
//  * `null` - for when the `contextValue` is non-boolean
if (contextValue === false || contextValue === null) {

However test would communicate it just as good and it would test it on top.

I have no preference, really, but I agree with @adamkudrna that one or the other should be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layout context not working for bool props
3 participants