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.

Sign up for GitHub

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

Jump to bottom

Integrate RN Nightly 6/13 #13346

Merged
merged 53 commits into from
Jul 10, 2024
Merged

Integrate RN Nightly 6/13 #13346

merged 53 commits into from
Jul 10, 2024

Conversation

Yajur-Grover
Copy link
Contributor

@Yajur-Grover Yajur-Grover commented Jun 14, 2024 •
edited
Loading

Description

Type of Change

Integration

What

facebook/react-native@ced0762...f7aea0c

Notable commits:
facebook/react-native@c92f31d
facebook/react-native@940d738
facebook/react-native@5b3a321
facebook/react-native@54997e4

Issue tracking reverting change of create-react-native-library package version in react-native-init-windows.yml: #13414

Changelog

No

Microsoft Reviewers: Open in CodeFlow

marlenecota
marlenecota reviewed Jun 25, 2024
View reviewed changes
...actCommon/TEMP_UntilReactCommonUpdate/react/nativemodule/core/ReactCommon/TurboModuleUtils.h Outdated

#include <ReactCommon/CallInvoker.h>
#include <CallbackWrapper.h> // [Windows]
Copy link
Contributor

Choose a reason for hiding this comment

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

Find out why we're forking this include. We either don't want to include <react/bridging/CallbackWrapper.h> or we can no unfork this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

And when you find the issue, add it to the comment please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue tracking CallbackWrapper.h and TurboModuleUtils.h forked files: #13399

marlenecota
marlenecota reviewed Jun 25, 2024
View reviewed changes
vnext/ReactCommon/ReactCommon.vcxproj Outdated
@@ -182,6 +182,7 @@
<ClInclude Include="$(ReactNativeDir)\ReactCommon\react\renderer\runtimescheduler\RuntimeScheduler_Modern.h" />
<ClInclude Include="$(ReactNativeDir)\ReactCommon\react\renderer\core\PropsParserContext.h" />
<ClCompile Include="$(ReactNativeDir)\ReactCommon\react\bridging\LongLivedObject.cpp" />
<ClInclude Include="$(ReactNativeDir)\ReactCommon\react\bridging\EventEmitter.h" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add this ClInclude in the middle of ClCompiles. Add EventEmitter.h under CallbackWrapper.h with the other ClIncludes.

@Yajur-Grover Yajur-Grover mentioned this pull request Jun 26, 2024
Open
@Yajur-Grover
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Pipelines were unable to run due to time out waiting for the pull request to finish merging.

@Yajur-Grover Yajur-Grover requested a review from a team as a code owner July 3, 2024 23:50
marlenecota
marlenecota reviewed Jul 4, 2024
View reviewed changes
.ado/templates/react-native-init-windows.yml Outdated
@@ -46,7 +46,7 @@ steps:

- ${{ if endsWith(parameters.template, '-lib') }}:
- script: |
npx --yes create-react-native-library@latest --slug testcli --description testcli --author-name "React-Native-Windows Bot" --author-email [email protected] --author-url http:https://example.com --repo-url http:https://example.com --languages java-objc --type module-new --react-native-version $(reactNativeDevDependency) testcli
npx --yes create-react-native-library@0.36.0 --slug testcli --description testcli --author-name "React-Native-Windows Bot" --author-email [email protected] --author-url http:https://example.com --repo-url http:https://example.com --languages java-objc --type module-new --react-native-version $(reactNativeDevDependency) testcli
Copy link
Contributor

Choose a reason for hiding this comment

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

Create issue to track this. Tag it as integration follow-up and link it in the PR description.

marlenecota
marlenecota requested changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@marlenecota marlenecota left a comment

Choose a reason for hiding this comment

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

Please add comments with tracking issues to the forked files.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Jul 4, 2024
acoates-ms
acoates-ms reviewed Jul 4, 2024
View reviewed changes
packages/e2e-test-app/test/__snapshots__/TextComponentTest.test.ts.snap Outdated
},
{
"Background": null,
"BorderBrush": "#FF008000",
"BorderThickness": "3,3,3,3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we lose support of borders on Text? - Or did the test change?

Yajur-Grover
Yajur-Grover commented Jul 4, 2024
View reviewed changes
vnext/src-win/Libraries/Text/Text.windows.js Outdated
} else {
let styleProps: ViewStyleProp = (restProps.style: any);
if (
styleProps &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acoates-ms this may be responsible for the change in borders for text? Not sure if this was intentional or a bad merge that I should fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you add this back in, does that restore the snapshot tests to the original?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so - but I think we have an extra if/else block in the Text.windows.js file which was removed in the merge which should not have been. This extra block includes the styleProps variable, so adding it back in should resolve the issue.

TatianaKapos
TatianaKapos reviewed Jul 9, 2024
View reviewed changes
packages/@office-iss/react-native-win32/src-win/Libraries/Text/Text.win32.js
<NativeVirtualText
{...restProps}
{...eventHandlersForText}
accessibilityLabel={_accessibilityLabel}
Copy link
Contributor

Choose a reason for hiding this comment

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

@acoates-ms Do we need the win32-specific properties here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we will - have updated the file accordingly. If not needed, will revert.

@Yajur-Grover Yajur-Grover enabled auto-merge (squash) July 10, 2024 01:12
@Yajur-Grover Yajur-Grover merged commit 8a91af5 into microsoft:main Jul 10, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acoates-ms acoates-ms acoates-ms left review comments

@marlenecota marlenecota marlenecota approved these changes

@TatianaKapos TatianaKapos TatianaKapos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Yajur-Grover @marlenecota @acoates-ms @TatianaKapos