-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[0.72 New Architecture] added missing text measure cache text attributes #37946
[0.72 New Architecture] added missing text measure cache text attributes #37946
Conversation
Hi @yungblud! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
I'm wondering |
Base commit: af57ce1 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@yungblud any specific reasons you are targetting 0.72-stable for this fix? I'd rather it point to main and we cherry-pick to 0.72, so that we ensure that the main codebase and future versions also have this fix. |
@kelset Oh, right. that is the right way. I will change base branch to main. |
Hmm too many file changes from main. |
44225ca
to
b82bb20
Compare
@kelset |
@@ -89,6 +89,9 @@ inline bool areTextAttributesEquivalentLayoutWise( | |||
// attributes that affect only a decorative aspect of displayed text (like | |||
// colors). | |||
return std::tie( | |||
lhs.foregroundColor, | |||
lhs.backgroundColor, | |||
lhs.opacity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does opacity, backgroundColor and foregroundColor change size/position of text? I'm not how it affects layout of text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I know TextMeasureCache is for size, position of Text component.
So I'm not sure this is the right way to fix style caching issue.
But it works.
Thank you very much for the PR. I eventually went with slightly different solution: #38070 I still need to test it in production. |
Summary:
Related issue
Changelog:
Added missing text measure cache text attributes
Pick one each for the category and type tags:
[IOS] [FIXED] - added missing text measure cache text attributes
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
Test Plan:
npx react-native@latest init RN0720RC1 --version 0.72.0-rc.6
cd RN0720RC1/ios
RCT_NEW_ARCH_ENABLED=1 bundle exec pod install --verbose