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

Rename ambiguous getLeading/Trailing... functions in Node.cpp #41018

Closed
wants to merge 2 commits into from

Conversation

joevilches
Copy link
Contributor

Summary:
X-link: facebook/yoga#1424

See D50344874

Differential Revision: D50344874

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Oct 16, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

@analysis-bot
Copy link

analysis-bot commented Oct 16, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,618,999 +2
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 20,992,562 -4
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 2018a82
Branch: main

joevilches added a commit to joevilches/yoga that referenced this pull request Oct 17, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018

Pull Request resolved: facebook#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: 1a24ae1cdb9374cd7b84895fb7d5ea6bc9507bb1
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

joevilches added a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:
Pull Request resolved: facebook#41018

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: 995bb1a6cd2e9a9d2b79caf8aab7e09152a9294c
joevilches pushed a commit to joevilches/yoga that referenced this pull request Oct 17, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

joevilches added a commit to joevilches/yoga that referenced this pull request Oct 17, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018

Pull Request resolved: facebook#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: a423a7f8cefbc541c1b0bb1dae11fe81cbedfe96
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

joevilches added a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:
Pull Request resolved: facebook#41018

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: fdff690712d7740072257afaa931a3b09ad79c16
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 17, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/yoga that referenced this pull request Oct 18, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 18, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

joevilches pushed a commit to joevilches/yoga that referenced this pull request Oct 18, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 18, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
Joe Vilches added 2 commits October 17, 2023 18:10
Summary:
X-link: facebook/yoga#1423


Before resolving facebook/yoga#1208 yoga was in a state where "leading" and "trailing" only referred to the main-start and main-end directions ([definition in spec](https://drafts.csswg.org/css-flexbox/#box-model)). That is, the start/end of the layout of flex items in a container. This is distinct from something like inline-start/inline-end which is the [start of text layout as defined by direction](https://drafts.csswg.org/css-writing-modes-3/#inline-start).

The bug linked above happened because "leading" and "trailing" functions are referring to the wrong directions in certain cases. So in order to fix this we added a new set of functions to get the "leading" and "trailing" edges according to what inline-start/inline-end would refer to - i.e. those defined by the direction (ltr | rtl). In this state I think it is confusing to understand which function refers to which direction and more specific names could help that.

This diff just renames the following 4 FlexDirection.h functions:

* **leadingEdge** -> **flexStartEdge**
* **trailingEdge** -> **flexEndEdge**
* **leadingLayoutEdge** -> **inlineStartEdge**
* **trailingLayoutEdge** -> **inlineEndEdge**

The spec calls the start/end directions as dictated by the flex-direction attribute "main-start" and "main-end" respectively, but mainStartEdge might be a bit confusing given it will be compared to a non-flexbox-specific name in inlineStartEdge. As a result I landed on flexStart/flexEnd similar to what values are used with alignment attributes (justify-content, align-content).

I chose to get rid of the "leading" and "trailing" descriptors to be more in line with what terminology the spec uses.

Next diff will be to rename the functions in Node.cpp to adhere to the above patterns.

Reviewed By: NickGerleman

Differential Revision: D50342254
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/yoga that referenced this pull request Oct 18, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50344874

joevilches pushed a commit to joevilches/yoga that referenced this pull request Oct 18, 2023
…ok#1424)

Summary:
X-link: facebook/react-native#41018


See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
joevilches pushed a commit to joevilches/react-native that referenced this pull request Oct 18, 2023
…ok#41018)

Summary:

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874
@github-actions
Copy link

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against b252c9c

facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Oct 18, 2023
Summary:
X-link: facebook/react-native#41018

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: 32fc35df674eb854c682a5e387c031a94c1c4f68
facebook-github-bot pushed a commit to facebook/yoga that referenced this pull request Oct 18, 2023
Summary:
X-link: facebook/react-native#41018

Pull Request resolved: #1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: 32fc35df674eb854c682a5e387c031a94c1c4f68
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 18, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in ef32905.

Othinn pushed a commit to Othinn/react-native that referenced this pull request Oct 30, 2023
…ok#41018)

Summary:
Pull Request resolved: facebook#41018

X-link: facebook/yoga#1424

See D50344874

Reviewed By: NickGerleman

Differential Revision: D50344874

fbshipit-source-id: 32fc35df674eb854c682a5e387c031a94c1c4f68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants