Skip to content

Commit

Permalink
Cleanup some multiline code
Browse files Browse the repository at this point in the history
Summary: some general cleanup

Reviewed By: lucasr

Differential Revision: D3886866

fbshipit-source-id: e19c1be4af58605933f90b5bf381008338be2236
  • Loading branch information
Emil Sjolander authored and Facebook Github Bot 5 committed Sep 20, 2016
1 parent 8fd14c7 commit 017fb2c
Showing 1 changed file with 36 additions and 29 deletions.
65 changes: 36 additions & 29 deletions CSSLayout/CSSLayout.c
Original file line number Diff line number Diff line change
Expand Up @@ -1700,16 +1700,17 @@ static void layoutNodeImpl(const CSSNodeRef node,
float lineHeight = 0;
for (ii = startIndex; ii < childCount; ii++) {
const CSSNodeRef child = CSSNodeListGet(node->children, ii);
if (child->style.positionType != CSSPositionTypeRelative) {
continue;
}
if (child->lineIndex != i) {
break;
}
if (isLayoutDimDefined(child, crossAxis)) {
lineHeight = fmaxf(lineHeight,
child->layout.measuredDimensions[dim[crossAxis]] +
getMarginAxis(child, crossAxis));

if (child->style.positionType == CSSPositionTypeAbsolute) {

This comment has been minimized.

Copy link
@jordwalke

jordwalke Oct 9, 2016

This logic looks different than the logic on the left, is that correct? This part is not just merely cleanup?

This comment has been minimized.

Copy link
@emilsjolander

emilsjolander Oct 9, 2016

Contributor

Yeah i accidently introduced a bug. It was fixed and a test was added in an future diff :)

This comment has been minimized.

Copy link
@jordwalke

jordwalke Oct 9, 2016

Yup, I found it 3b1515f

This comment has been minimized.

Copy link
@jordwalke

jordwalke Oct 9, 2016

It's weird I have those tests, and applying that fix makes no difference to those particular test cases (maybe my test suite is broken?)

This comment has been minimized.

Copy link
@emilsjolander

emilsjolander via email Oct 9, 2016

Contributor
if (child->lineIndex != i) {
break;
}

if (isLayoutDimDefined(child, crossAxis)) {
lineHeight = fmaxf(lineHeight,
child->layout.measuredDimensions[dim[crossAxis]] +
getMarginAxis(child, crossAxis));
}
}
}
endIndex = ii;
Expand All @@ -1718,26 +1719,32 @@ static void layoutNodeImpl(const CSSNodeRef node,
if (performLayout) {
for (ii = startIndex; ii < endIndex; ii++) {
const CSSNodeRef child = CSSNodeListGet(node->children, ii);
if (child->style.positionType != CSSPositionTypeRelative) {
continue;
}

const CSSAlign alignContentAlignItem = getAlignItem(node, child);
if (alignContentAlignItem == CSSAlignFlexStart) {
child->layout.position[pos[crossAxis]] =
currentLead + getLeadingMargin(child, crossAxis);
} else if (alignContentAlignItem == CSSAlignFlexEnd) {
child->layout.position[pos[crossAxis]] =
currentLead + lineHeight - getTrailingMargin(child, crossAxis) -
child->layout.measuredDimensions[dim[crossAxis]];
} else if (alignContentAlignItem == CSSAlignCenter) {
childHeight = child->layout.measuredDimensions[dim[crossAxis]];
child->layout.position[pos[crossAxis]] = currentLead + (lineHeight - childHeight) / 2;
} else if (alignContentAlignItem == CSSAlignStretch) {
child->layout.position[pos[crossAxis]] =
currentLead + getLeadingMargin(child, crossAxis);
// TODO(prenaux): Correctly set the height of items with indefinite
// (auto) crossAxis dimension.
if (child->style.positionType == CSSPositionTypeAbsolute) {
switch (getAlignItem(node, child)) {
case CSSAlignFlexStart:
child->layout.position[pos[crossAxis]] =
currentLead + getLeadingMargin(child, crossAxis);
break;
case CSSAlignFlexEnd:
child->layout.position[pos[crossAxis]] =
currentLead + lineHeight - getTrailingMargin(child, crossAxis) -
child->layout.measuredDimensions[dim[crossAxis]];
break;
case CSSAlignCenter:
childHeight = child->layout.measuredDimensions[dim[crossAxis]];
child->layout.position[pos[crossAxis]] =
currentLead + (lineHeight - childHeight) / 2;
break;
case CSSAlignStretch:
child->layout.position[pos[crossAxis]] =
currentLead + getLeadingMargin(child, crossAxis);
// TODO(prenaux): Correctly set the height of items with indefinite
// (auto) crossAxis dimension.
break;
default:

This comment has been minimized.

Copy link
@jordwalke

jordwalke Oct 9, 2016

Likewise, what about auto? What should happen, and is there currently a bug?

This comment has been minimized.

Copy link
@emilsjolander

emilsjolander Oct 9, 2016

Contributor

Auto doesn't need to be handled here as its an alias for stretch

This comment has been minimized.

Copy link
@jordwalke

jordwalke Oct 10, 2016

It looks like align-self defaults to auto, which pretty much means inherit from parent, and align-items on the parent defaults to stretch.

Also interesting, it looks like the spec says that align-self can be auto, but align-items cannot be auto.

http://www.w3schools.com/cssref/css3_pr_align-self.asp
http://www.w3schools.com/cssref/css3_pr_align-items.asp

So that means, yeah getAlignItem could never really return auto. I might even throw if we discovered it was the case.

break;
}
}
}
}
Expand Down

0 comments on commit 017fb2c

Please sign in to comment.