-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Jakubkalinski0/fix console errors related to forward ref batch8 #72072
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
Jakubkalinski0/fix console errors related to forward ref batch8 #72072
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…-forwardRef-batch8
…-forwardRef-batch8
…-forwardRef-batch8
…-forwardRef-batch8
…-forwardRef-batch8
|
@aimane-chnaif 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] |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Screenshots/VideosComposer1-Composer.mov1-Composer-native.movEmojiReactionBubble6-EmojiReactionBubble.movRoomNameInput7-RoomNameInput.mov7-RoomNameInput-native.movSelectionListWithModal8-SelectionListWithModal.movSingleFieldStep9-SingleFieldStep.movTextPicker10-TextPicker.movTimePicker11-TimePicker.movValidateCodeActionForm, ContactMethodDetailsPage12-ValidateCodeActionForm.ContactMethodDetailsPage.movValuePicker, withNavigationFallback13-ValuePicker.withNavigationFallback.movAvatarCapture14-AvatarCapture.mov14-AvatarCapture-native.mov |
src/components/LHNOptionsList/OptionRowRendererComponent/index.native.tsx
Show resolved
Hide resolved
|
@jakubkalinski0 can you please confirm and resolve all the comments from @aimane-chnaif please? thanks! |
|
I will Respond to the comments in the evening today, as I have some personal matters to attend to. Although I see that those comments regard only unused refs which we will take care of in the followup alongside the unused refs from the batch 7, so maybe we can move forward with this PR anyway? |
|
Seems like we need to fix eslint though |
|
Oh, let's do this in separate PR. Looks like it's used in many files. |
|
Oh sorry, I completely missed that eslint error 😅 I will handle it in a minute I will also merge newer main |
|
Responded to all comments - I suggest to take care of them in the cleanup PR with unused refs from batch 7 |
mountiny
left a comment
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.
Thanks
src/components/ValidateCodeActionModal/ValidateCodeForm/index.android.tsx
Show resolved
Hide resolved
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.64-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.64-5 🚀
|





Explanation of Change
The whole issue comes from
forwardRefbeing deprecated in the near future of react thus we will no longer be able to access the passedrefbyelement.refasforwardRefis needed for passingrefas a separate attribute. Now in order to pass there reference it has to be passed as one of thepropsso we can access it directly or like that ->element.props.ref.In order to adjust the codebase to new react recommendations in every file that was using
forwardRefthe ref had to be moved to thepropsinstead of being a separate attribute andforwardRefhad to be completely removed.Some files had to be changed due to the changes made in other components. In response to the number of files that needed adjusting the changes are made in batches with this one being
the eighth one.This batch is pretty
scattered all over the placejust as the previous one due to there being no more clear flows that can provide us with meaningful number of files that require the change.Modified files are grouped below by the components they are related to. In order to make testing easier there are also screenshots of places in app where you can find those components.
Composer
GrowlNotification
!!! I couldn't find a single place where this component can be naturally seen by a user - it's only present in Expensify.tsx but it doesn't seem to me like it's triggered anywhere. I suggest to remove it in a followup !!!
LHNOptionsList
Lottie
Onfido
PopoverWithoutOverlay
Reactions/EmojiReactionBubble
RoomNameInput
SelectionListWithModal
SubStepForms/SingleFieldStep
TextPicker
TimePicker
ValidateCodeActionForm
ValidateCodeForm
ValuePicker
withNavigationFallback
It's display name differs hence, thats why you see screenshot of WithNavigationFocusWithFallback

AvatarCapture
ContactMethodDetailsPage
AccountValidatePage
NO QA FOR THE CHANGES MADE IN THE FILES BELOW
tests/ui
__mocks__
Performance
Fixed Issues
$ #67033
PROPOSAL: N/A
Tests
LottiePeople you may know are already here! Verify your email to join them.screen. (I believe there have to be workspaces created within your organization i.e. in my case it's a workspace created by someone with@swmansion.comdomain, but I suppose that shouldn't be a problem) - testingValidateCodeFormFloating Action Buttonin the bottom left corner -> then pressStart chatand choose# RoomRoomNameInputValuePickerComposeremoji picker menu; verify that the menu works properly and if you can choose any emoji - testingPopoverWithoutOverlayEmojiReactionBubbleLHNOptionsListMemberssection and verify that you can navigate through the list of members properly - testingSelectionListWithModalFeaturesand turn onTaxesand then go toTaxesAdd ratein the top right corner then verify if naming this rate works properly - testingTextPickerAccount->ProfileStatus, input any message and press theClear afterrow -> choose custom. Then change the time and verify that it works properly (you can verify if the status is automatically dropped after the set time, although that isn't influenced by the changes of this PR) - testingTimePickerAvatarCaptureContact methodsrow and press on any of your contact methods to verify if the details about this contact show up properly - testingContactMethodDetailsPageSecurityand pressMerge accountsrow, input any account and pressNext. Verify that the next screen asking for the magic code sent to another account opens up properly - testingAccountValidatePageValidateCodeActionFormWalletEnable walletphone numberverify that it works correctly and that the phone number is properly saved on the confirmation page - testingSingleFieldStepOnfidoverification properly shows up and responds correctly to your actions (there is no possibility for me to actually pass the Onfido verification - it's very good at it's job) - testingOnfidoOffline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Steps from 1 to 5:
1to5.mp4
Steps from 6 to 9:
6to9.mp4
Steps from 10 to 12:
10to12.mp4
Steps from 13 to 17:
13to17.mp4
Steps from 18 to 23:
18to23.mp4
Steps from 24 to 28:
24to28.mp4
MacOS: Desktop