Skip to content

Migrated file ReactStylesDiffMap to kotlin #50616

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

poonamjain96
Copy link
Contributor

@poonamjain96 poonamjain96 commented Apr 10, 2025

Summary:

This PR aims to migrate ReactStylesDiffMap from Java to kotlin as part of #50513

Changelog:

[ANDROID][CHANGED]Migrate ReactStylesDiffMap to Kotlin

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Test Plan:

Tested on RN tester with both new and old arch

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Apr 10, 2025
@poonamjain96 poonamjain96 changed the title fix: migrated to kotlin code Migrated ReactStylesDiffMap.java to kotlin code Apr 10, 2025
@poonamjain96 poonamjain96 changed the title Migrated ReactStylesDiffMap.java to kotlin code Migrated file ReactStylesDiffMap to kotlin Apr 10, 2025
@cortinico cortinico requested a review from mateoguzmana April 10, 2025 08:07
Copy link
Collaborator

@mateoguzmana mateoguzmana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! it looks generally good, some very small things to check

Copy link
Collaborator

@mateoguzmana mateoguzmana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for the internal reviewer: This class has some OSS usages, so removing mBackingMap hungarian notation would break OSS, probably that has to be suppressed if the linter fails

@mateoguzmana mateoguzmana requested a review from cortinico April 11, 2025 14:31
@cortinico
Copy link
Contributor

Note for the internal reviewer: This class has some OSS usages, so removing mBackingMap hungarian notation would break OSS, probably that has to be suppressed if the linter fails

Oh really? What are those @mateoguzmana ?

@mateoguzmana
Copy link
Collaborator

Note for the internal reviewer: This class has some OSS usages, so removing mBackingMap hungarian notation would break OSS, probably that has to be suppressed if the linter fails

Oh really? What are those @mateoguzmana ?

This is the GH search

But, I may be misinforming by saying that mBackingMap is used (I could not find direct references for that in some of the initial results), but given how many libs use the class, I'd still go on the safe path

@cortinico
Copy link
Contributor

@mateoguzmana given the last GH search, I think we can actually rename the field and make it internal/private

@mateoguzmana
Copy link
Collaborator

@mateoguzmana given the last GH search, I think we can actually rename the field and make it internal/private

Interesting, thanks for checking!

Copy link
Collaborator

@mateoguzmana mateoguzmana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing before it can be imported

@mateoguzmana mateoguzmana requested review from cortinico and removed request for cortinico April 11, 2025 18:35
@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants