Change flex getters to return the set values #431
Conversation
tests/YGDefaultValuesTest.cpp
Outdated
| ASSERT_FLOAT_EQ(0, YGNodeStyleGetFlexGrow(root)); | ||
| ASSERT_FLOAT_EQ(0, YGNodeStyleGetFlexShrink(root)); | ||
| ASSERT_FALSE(YGNodeStyleGetFlexBasis(root).unit != YGUnitAuto); | ||
| ASSERT_TRUE(YGFloatIsUndefined(YGNodeStyleGetFlexGrow(root))); |
There was a problem hiding this comment.
I think these should still return 0 (their current default) if nothing has been set.
There was a problem hiding this comment.
Currently our internal default is Undefined and we use that value inside YGResolveFlexGrow to check if explicitly set. What do you think how we should use that with the flex shorthand? Should we use flex to set the flexGrow directly. But so we loose the previous set values.
There was a problem hiding this comment.
As flex
is a shorthand for the flex-grow, flex-shrink, and the flex-basis properties.
maybe we should explictily set flex-grow, flex-shrink if we call it?
There was a problem hiding this comment.
An alternative is, to return 0 for these values in a (custom) getter, and keep the undefined as an internal implementation detail?
yoga/Yoga.c
Outdated
| YG_NODE_STYLE_PROPERTY_SETTER_IMPL(float, FlexGrow, flexGrow, flexGrow); | ||
| YG_NODE_STYLE_PROPERTY_SETTER_IMPL(float, FlexShrink, flexShrink, flexShrink); | ||
| YG_NODE_STYLE_PROPERTY_SETTER_UNIT_AUTO_IMPL(float, FlexBasis, flexBasis, flexBasis); | ||
| YG_NODE_STYLE_PROPERTY_IMPL(float, FlexGrow, flexGrow, flexGrow); |
There was a problem hiding this comment.
Can you also add a YG_NODE_STYLE_PROPERTY_IMPL for the Flex shorthand?
|
👍 |
|
@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
yoga/Yoga.c
Outdated
| @@ -423,7 +423,15 @@ inline float YGNodeStyleGetFlexGrow(const YGNodeRef node) { | |||
| return 0.0f; | |||
| } | |||
|
|
|||
| inline float YGNodeStyleGetFlexShrink(const YGNodeRef node) { | |||
| float YGNodeStyleGetFlexGrow(const YGNodeRef node) { | |||
| return YGFloatIsUndefined(node->style.flexGrow) ? 0.0f : node->style.flexGrow; | |||
There was a problem hiding this comment.
Instead of 0.0f, can we have the reference the defaults instead for clarity and to not introduce bugs?
|
We don't really have a default constant here. But this return value is at least covered by the default tests. |
|
@astreet Right, I forgot about this. The value encoded in the default struct (gYGNodeDefaults) is |
|
@woehrl01 Can you pull out 0.0f into constants kDefaultFlexGrow and kDefaultFlexShrink ? then use these in both the style getter and the resolve function? I think that would make it clearer what the zero is referencing. |
|
Sounds like a great idea 💡 will do! |
woehrl01
changed the title
woehrl01
changed the title
|
@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Changed the flex getters to return the values they were actually set. See #421 . Closes facebook/yoga#431 Reviewed By: astreet Differential Revision: D4604744 Pulled By: emilsjolander fbshipit-source-id: 02d79100ef22be866db1c3bd9d53e4447186811f
Summary: Changed the flex getters to return the values they were actually set. See facebook#421 . Closes facebook/yoga#431 Reviewed By: astreet Differential Revision: D4604744 Pulled By: emilsjolander fbshipit-source-id: 02d79100ef22be866db1c3bd9d53e4447186811f
Summary: Changed the flex getters to return the values they were actually set. See facebook#421 . Closes facebook/yoga#431 Reviewed By: astreet Differential Revision: D4604744 Pulled By: emilsjolander fbshipit-source-id: 02d79100ef22be866db1c3bd9d53e4447186811f
Summary: Changed the flex getters to return the values they were actually set. See #421 . Closes facebook/yoga#431 Reviewed By: astreet Differential Revision: D4604744 Pulled By: emilsjolander fbshipit-source-id: 02d79100ef22be866db1c3bd9d53e4447186811f
Changed the flex getters to return the values they were actually set. See #421 .