-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[No QA] Console error cleanup pt3 #55066
base: main
Are you sure you want to change the base?
[No QA] Console error cleanup pt3 #55066
Conversation
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
… Url cannot be created from undefined
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
@dukenv0307 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@kubabutkiewicz How to reproduce these console errors?
|
Reviewer Checklist
Screenshots/VideosAndroid: Native
Screen.Recording.2025-02-03.at.23.22.30.mov
Screen.Recording.2025-02-07.at.22.59.57.mov
Screen.Recording.2025-02-07.at.23.00.40.movAndroid: mWeb ChromeiOS: Native
Screen.Recording.2025-02-03.at.23.16.34.mov
Screen.Recording.2025-02-07.at.22.46.10.mov
Screen.Recording.2025-02-07.at.22.50.04.moviOS: mWeb SafariMacOS: Chrome / SafariBefore
![]()
![]()
After
TextBlock - Each child in a list should have a unique 'key' prop SearchRouter - Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()? web-resize.mp4
![]() MacOS: Desktopweb-resize.mp4 |
@kubabutkiewicz Can you please add the screenshot/video? This issue is not fixed
![]() |
Was happening when I was entering channel called #admins on any workspace
Was happening when opening attachments on native I will take a look at the failed error |
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
@dukenv0307 For me the about issue with react-native-gesture-handler we want to just get rid of those errors which are on the screenshot you posted. I also added the fix for failed error |
I pushed change @dukenv0307 but I was asked if I can include one more fix #55045 (comment) which I need some time to fix so its not ready to review |
…to console-error-cleanup-pt3
@kubabutkiewicz Thank you. BTW, can you please add the detail on the PR description about what we fixed, it helps QA easily to follow |
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
|
console.log("## nested draggable list measure fail"); | ||
}; | ||
+ | ||
+ if (typeof nodeHandle === "number" ) { |
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.
Can you explain about this change?
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.
This is basically taken from react-native codebase https://github.com/facebook/react-native/blob/fffd6d75b48ae1f6c78dbc0f7e9b6fe509df3394/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactFabricHostComponent.js#L91-L99 no not call this method without proper type.
Code looks good and tests well. I just left 1 minor question. And can you please mark this task as done?
|
@kubabutkiewicz any updates? |
Sorry missed your comment, answered. |
@kubabutkiewicz Can you please merge main, I think the failing test is fixed |
…to console-error-cleanup-pt3
…to console-error-cleanup-pt3
@dukenv0307 done but I think the workflow isnt fixed yet, PR which I mentioned earlier isnt merged yet |
@kubabutkiewicz That's weird, I don't see any problems on some recent PRs. For example: #56678 |
@mountiny can you help with checking why the VerifyHybridApp is failing? |
@dukenv0307 I think I know what an issue, this workflow is only running when we are changing one of the files on this paths
but the workflow is broken thats why its failing |
@kubabutkiewicz Agree, I think this PR should be merged |
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.
LGTM
We did not find an internal engineer to review this PR, trying to assign a random engineer to #55045 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
…to console-error-cleanup-pt3
🚧 @lakchote has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
…to console-error-cleanup-pt3
🚧 @lakchote has triggered a test hybrid app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Explanation of Change
Fixed console errors in that issue:
validateDOMNesting(...): <button> cannot appear as a descendant of <button>.
Fixed Issues
$ #55045
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop