Baseline support #317
Assignees
DaddyFrosty
Clear assignees
DaddyFrosty
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: DaddyFrosty/yoga#317
Reference in New Issue
Block a user
No description provided.
Delete Branch "baseline-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Added baseline support (see #132)
You have the ability for a custom baseline function (
float(*YGBaselineFunc)(YGNodeRef node);) to return whatever baseline you want.Not sure if we should cache the baseline, as we get the baseline twice. First to get the maximum and second to position it.
I'm thinking about something like
computedFlexBasisGenerationjust namedbaselineCalculationGenerationand a cached value property.This is awesome! Thanks! I'll do a more thorough review later this week. Please run format.sh in the mean time.
@@ -334,6 +335,14 @@ YGMeasureFunc YGNodeGetMeasureFunc(const YGNodeRef node) {return node->measure;node->baseline = baselineFunc;is enough@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}}uint32_t
i++
I'd love to do this, but the effort to create the environment to run this on windows, is currently too much work for me. As it seems I need a full linux runtime to execute this.
@woehrl01 having clang installed should be enough 👍 But that's fine, I can run it before merging. Could you try to adhere to the code style of the rest of the project though?
@emilsjolander clang to the rescue! 👍 strangely clang-format on windows doesn't allow wildcard. So I hope I haven't missed a file.
@woehrl01 That's not clang, most likely your shell is not doing the expansion.
true 😄
Please add some tests for column direction as well as row direction. Also please add tests for align-self usage.
@@ -2426,3 +2490,4 @@!YGFloatIsUndefined(availableInnerCrossDim)) {const float remainingAlignContentDim = availableInnerCrossDim - totalLineCrossDim;float crossDimLead = 0;performLayoutcheck is not needed here as it is already checked in the containing if statement@@ -933,3 +946,4 @@return align;}static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,YGNodeBaseline@@ -2426,3 +2490,4 @@!YGFloatIsUndefined(availableInnerCrossDim)) {const float remainingAlignContentDim = availableInnerCrossDim - totalLineCrossDim;float crossDimLead = 0;isn't performlayout checked at line 2526 a little bit down below?
YGAlignBaselineCustomTest.cpp->YGBaselineFuncTest.cpp?please test with different widths on the two items as width is the axis being aligned here
indentation
indendation
please vary the widths as this is a column layout
This whole file would probably make sense to have as part of the YGAlignItems.html + YGAlignSelf.html files which already exist
@@ -929,7 +938,12 @@ static inline float YGNodePaddingAndBorderForAxis(const YGNodeRef node,}const
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}This will always be height as baseline is undefined for column flex direction. right? in that case I think it is better to be clear and use
YGDimensionHeightinstead ofdim[crossAxis]Is there a good reason to allow undefined return value for baseline? I would prefer asserting and crashing as it would probably indicate a bug. What do you think?
Why
child->lineIndex > 0? I understand that it skips multiline children but i'm not sure why.same as above regarding
dim[crossAxis]The baseline of a container is defined by its first baseline aligned child, but when there is no baseline aligned child then the container's baseline is defined by it's last child. This seems odd. Are you sure we have test cases covering edge cases here to make sure this is how it works on the web?
same as above regarding
pos[crossAxis]@@ -966,6 +1014,24 @@ static inline bool YGNodeIsFlex(const YGNodeRef node) {(YGNodeStyleGetFlexGrow(node) != 0 || YGNodeStyleGetFlexShrink(node) != 0));}nit: empty line above
same as above. Don't reference
crossAxisas it confuses the fact that baseline is only defined for row layouts@@ -49,6 +49,7 @@ typedef YGSize (*YGMeasureFunc)(YGNodeRef node,YGMeasureMode widthMode,nit: remove added empty lines
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}I'm not sure my self about this. I thought this could be a kind of "feature", so if you return undefined, you simply use the nodes height. But crashing would be fine too for me. What I'm not sure about is if we explicitly add the padding-top here or if the implementation of the custom function needs to consider this.
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}we use only the first line of the children for base layout alignment. At least this is how chrome handles it.
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}no it's defined by its first child on the first line or the first baseline aligned child if there is one (one the first line). I'll add a test for this.
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}The custom function should not take padding into account. We don't expect this for the measure function so I would like to preserve that here if possible.
Let's crash for now. If we find a valid reason to have this feature we can implement it later.
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}yes, but this looks like we are skipping over children with multiple lines instead of just looking at their first line?
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}So the current code is wrong i think. Right? As it will pick the last child in the case no child is baseline aligned?
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}oh, yep. We should break instead of continue here!
@@ -941,6 +955,40 @@ static inline YGDirection YGNodeResolveDirection(const YGNodeRef node,}no as we only use the child if
baselineChildis stillNULL. Which is false as seen as we find the first one. We still need to iterate to take any baseline aligned child into account.@woehrl01 If you push all updates in one go (Ok if they are different commits) that would make it more clear to me and others when you are finished addressing comments so we can have another look 👍
should I use
YGFlexDirectionColumnhere on theYGNodeLeadingMarginandYGNodeMarginForAxiscalls?@emilsjolander sorry, will do next time!
yes :) and replace
dim[crossAxis]I think I got all your points, please have a look 😄
not quite sure why we break if a child has more than one line. That would mean if the first child of this node has two lines then the baseline would default to the height of the node. I'm not sure why that would be correct.
@@ -966,6 +1014,24 @@ static inline bool YGNodeIsFlex(const YGNodeRef node) {(YGNodeStyleGetFlexGrow(node) != 0 || YGNodeStyleGetFlexShrink(node) != 0));}Please add this 👍
Could
pos[crossAxis]be hardcoded? Same withcrossAxisa couple lines below@@ -49,6 +49,7 @@ typedef YGSize (*YGMeasureFunc)(YGNodeRef node,YGMeasureMode widthMode,nit: remove added empty line
we break as only childs on the first line which have align baseline are taken into account. I already added a testcase for this.
@@ -966,6 +1014,24 @@ static inline bool YGNodeIsFlex(const YGNodeRef node) {(YGNodeStyleGetFlexGrow(node) != 0 || YGNodeStyleGetFlexShrink(node) != 0));}not sure what you mean, should I add a empty line above
return false?sure, I just wanted to be the same like the others, to not cause confusion. But I'll replace them, to be explicit that it is only used on
YGEdgeTopoh, I see the confusion,
child->lineIndexdoes not mean that it has more than one line. It means it is itself on the seconds (or later) line.@woehrl01 yeah that was my confusion, i'm not super familiar with the multi line code and read it too quickly 🐱
@emilsjolander I additionally modified
YGIsBaselineLayoutto shortcut to false if it's a column layout.@emilsjolander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@woehrl01 The baseline feature is really awesome, but I'm not sure how to implement the function correctly. Would you give me some suggestions? Thanks.
@anql: Sure! You must return the vertical position inside the node you want to align.
If you do not have a baseline function set, it uses the nodes height. That means that all the nodes are align at their bottom line.
If you have a text node where you want to align your nodes across each others, you need to return the position of the baseline of the firstline inside the node.
I unpacked my "awesome" paint skills 😂, to make this a bit clearer:
If the black numbers are the height of the nodes, you need to return the red values, to align the nodes across. If you have multiline text you need to return the value of the first line.
The paint is so clearer, it's very helpful for me. 👍 @woehrl01
Pull request closed