When paddingStart is 0, it should override paddingHorizontal#816
Closed
rigdern wants to merge 2 commits intofacebook:masterfrom
Closed
When paddingStart is 0, it should override paddingHorizontal#816rigdern wants to merge 2 commits intofacebook:masterfrom
rigdern wants to merge 2 commits intofacebook:masterfrom
Conversation
Fixes facebook#815 ### Problem Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. ### History It looks like facebook@3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. ### Testing I manually verified this using the code in facebook#815. I'm not sure how to write a test for this since it requires setting `paddingHorizontal` which isn't supported on the web. AFAIK, all of the Yoga tests are code-generated based on web browser behavior.
rigdern
changed the title
Contributor
|
Hi Adam, great finding! Not every test is generated from the web, I think |
Author
|
@woehrl01 thanks for the suggestion. I added some unit tests to catch this bug and other similar issues. |
Contributor
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
pushed a commit
to facebook/litho
that referenced
this pull request
Oct 12, 2018
Summary: Fixes #815 Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works). It looks like facebook/yoga@3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues. Adam Comella Microsoft Corp. Pull Request resolved: facebook/yoga#816 Reviewed By: priteshrnandgaonkar Differential Revision: D10282617 Pulled By: shergin fbshipit-source-id: 2ab2874ae39d9454308a020a960ace85573fe777
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 12, 2018
Summary: Fixes #815 Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works). It looks like facebook/yoga@3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues. Adam Comella Microsoft Corp. Pull Request resolved: facebook/yoga#816 Reviewed By: priteshrnandgaonkar Differential Revision: D10282617 Pulled By: shergin fbshipit-source-id: 2ab2874ae39d9454308a020a960ace85573fe777
t-nanava
pushed a commit
to microsoft/react-native-macos
that referenced
this pull request
Jun 17, 2019
Summary: Fixes #815 Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works). It looks like facebook/yoga@3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues. Adam Comella Microsoft Corp. Pull Request resolved: facebook/yoga#816 Reviewed By: priteshrnandgaonkar Differential Revision: D10282617 Pulled By: shergin fbshipit-source-id: 2ab2874ae39d9454308a020a960ace85573fe777
CodeWitchBella
pushed a commit
to CodeWitchBella/yoga-wasm
that referenced
this pull request
Sep 2, 2019
Summary:
Fixes #815
Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`.
After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`.
Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works).
It looks like facebook/yoga@3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`.
I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues.
Adam Comella
Microsoft Corp.
Pull Request resolved: facebook/yoga#816
Reviewed By: priteshrnandgaonkar
Differential Revision: D10282617
Pulled By: shergin
fbshipit-source-id: 2ab2874ae39d9454308a020a960ace85573fe777
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #815
Problem
Imagine a node with this style:
{ paddingHorizontal: 10, paddingStart: 0 }.After running layout on this node, we expect its computed
paddingStartto be0. However, it is actually10.Fix
Consider the expression
paddingEdgeStart.getValue() > 0.0fingetLeadingPadding. Why is0handled like a negative number rather than a positive number? I suspect this should be>=so0is handled like the positive numbers (this is howgetTrailingPaddingworks).History
It looks like 3a82d2b?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from
>=to>ingetLeadingPadding. I suspect it was a mistake.getTrailingPaddingstill uses>=.Testing
I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues.
Adam Comella
Microsoft Corp.