-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Jakubkalinski0/fix console errors related to forward ref batch7 - PR after second revert #73422
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 batch7 - PR after second revert #73422
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
review started already. tons of test cases 😓 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
(Note to myself) Previous regressions:
@jakubkalinski0 please let me know if you're aware of any other regressions to be taken care of. |
|
@aimane-chnaif how is this looking? |
aimane-chnaif
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.
Test is still ongoing. Around 10 test cases tested so far.
| import useActionSheetKeyboardSpacing from './useActionSheetKeyboardSpacing'; | ||
|
|
||
| const ActionSheetAwareScrollView = forwardRef<ScrollView, ActionSheetAwareScrollViewProps>(({style, children, ...props}, ref) => { | ||
| function ActionSheetAwareScrollView({style, children, ref, ...props}: ActionSheetAwareScrollViewProps) { |
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.
I don’t see the benefit of this component anywhere.
i.e. #54764 test case is not working at all. (Also on main)
Can you find any example case where animation is working?
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.
If this is not used, lets create an issue to remove it
JmillsExpensify
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.
No review require from the product team. Unsubscribing.
Screenshots/VideosAddressSearchAddressSearch.movAddressSearch-ios.movAmountPickerAmountPicker.movAmountPicker-ios.movAmountWithoutCurrencyInputAmountWithoutCurrencyInput.movAmountWithoutCurrencyInput-ios.movAttachmentCarousel/Pager-iosAttachmentCarousel.Pager-ios.movCheckboxWithLabelCheckboxWithLabel.movCheckboxWithLabel-ios.movContextMenuItemContextMenuItem.movContextMenuItem2.movCountryStateSelectorCountryStateSelector.movCountryStateSelector-ios.movCurrencySelectorCurrencySelector.movDatePickerDatePicker.movHighlightableMenuItemHighlightableMenuItem.movInteractiveStepInteractiveStep.movInteractiveStepSubHeaderInteractiveStepSubHeader.movMagicCodeInputMagicCodeInput.movMagicCodeInput-ios.movPercentageFormPercentageForm.movPercentageForm-ios.movRadioButtonsRadioButtons.movShareTabShareTab-2.movShareTab-1.movTextLinkTextLink.movTextLink-ios.movTimeModalPickerTimeModalPicker.movTimeModalPicker-ios.movwithToggleVisibilityViewwithToggleVisibilityView.movwithToggleVisibilityView2.movwithToggleVisibilityView-ios.movwithViewportOffsetTopwithViewportOffsetTop.mov |
Reviewer Checklist
|
aimane-chnaif
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.
Tests pass on all affected pages.
All above comments are non blockers so approving
|
Sorry for not responding guys, everybody was OOO on the 11th here 😅 |
Nope, those are the only ones that I am aware of. Also the second one turned out to not be a regression at all if I remember correctly (it simply worked like that and nobody ever noticed). |
|
Sorry for so many test cases by the way but in those later batches it's almost impossible to find one flow which could cover any meaningful amount of components. I tried to at least write those tests as clearly as possible 😮💨 |
|
I looked into some of your comments but will take care of all of them properly tomorrow. I am not working long enough today and want to be precise. |
|
Let's keep all unused refs as is for now, as long as they exist on main. (to avoid unexpected regressions) |
|
We can do that, I will still reply to your comments here simply for clarity's sake |
|
Great job with this review, by the way ❤️ I like my reviews thorough |
| import useActionSheetKeyboardSpacing from './useActionSheetKeyboardSpacing'; | ||
|
|
||
| const ActionSheetAwareScrollView = forwardRef<ScrollView, ActionSheetAwareScrollViewProps>(({style, children, ...props}, ref) => { | ||
| function ActionSheetAwareScrollView({style, children, ref, ...props}: ActionSheetAwareScrollViewProps) { |
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.
If this is not used, lets create an issue to remove it
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.59-0 🚀
|
|
@mountiny Technically |
|
I replied to all the comments and we will definitely need a cleanup PR. I suggest we do it after the batch 8, to also take care of the things that surface there. |
|
@jakubkalinski0 @mountiny Can you help with 27th step? We can't open this link https://swmansion.slack.com/archives/C01GTK53T8Q/p1711049879962529 |
|
@IuliiaHerets I am not sure if I am allowed to copy this info here (honestly I doubt it), can you reach out to me on slack? |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.59-5 🚀
|



@mountiny
Raised once again after it was reverted.
Original PR -> #70878
Revert -> #71669
Revert number 2 -> #72106
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 seventh 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, thus all PR's going forward will probably be this scattered.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.
ActionSheetAwareScrollView
This one we test indirectly as it is used by other components
AddressSearch
This one has as different display name
AmountPicker
AmountWithoutCurrencyInput
AttachmentCarousel/Pager
This one is only on native:
CheckboxWithLabel
ContextMenuItem
CountrySelector
CurrencySelector
DatePicker
FormScrollView
HighlightableMenuItem
InteractiveStep
MagicCodeInput
PercentageForm
RadioButtons
ShareTabParticipantsSelector
This one is only on native:
SingleChoiceQuestion
StateSelector
TextLink
TimeModalPicker
withToggleVisibilityView
This one has a display name a little different than it's called, so don't get surprised
withViewportOffsetTop
Fixed Issues
$ #67033
PROPOSAL: N/A
Tests
MagicCodeInputandwithToggleVisibilityViewcomponents)BaseLoginForm- with that we testwithToggleVisibilityViewwhich is used by theBaseLoginFormContinueMagicCodeInputworks properly by pressingSign inafter inputing the codeAccount -> ProfileAddressinPrivatesection (now we will be testingAddressSearch,CountrySelectorandStateSelectorcomponents)AddressSearchworks properly (i.e. you can look up any address)CountrySelectorUnited StatesStateSelectorSaveDate of birthinPrivatesection (now we will be testingDatePickercomponent)DatePickerworks by checking if the picked date shows up in theDateinput on topSaveSubscriptionAdd payment card(now we will be testingCurrencySelectorcomponent)CurrencySelectorworks correctly by pressing it and changing the currencySecurityMerge accounts(now we will be testingCheckboxWithLabel)CheckboxWithLabelworks correctly by pressing it (you can do it multiple times)WalletEnable wallet(now we will be testingInteractiveStepSubHeader,InteractiveStepWrapper,RadioButtonsandSingleChoiceQuestioncomponents)InteractiveStepWrapperworks properly by filling out the bank infoInteractiveStepSubHeaderworks properly by checking if the step header 'checks' the first step and moves on to focus the second one after filling out the bank infoPersonalInfosection using information from the above slack discussion but it is for some reason crucial that you input the last name as Bobbeth otherwise you won't seeIdologyQuestionswhich we need to testRadioButtonsandSingleChoiceQuestioncomponents work properly by answering the questions (you can do that correctly or not or both ways to test it - testing it with only one of those is fine and completely enough)Workspacesand choose anyworkspacethat you are an owner ofMore -> Share(now we will be testing ifContextMenuItemcomponent works properly)ContextMenuItemworks properly by pressing theCopy URLand pasting it in the search bar of the web browserHighlightableMenuItemworks properly by going to any feature of the workspace - if you can navigate between the features of the workspace (for exampleOverviewtoMore features) then it worksMore featuresand turn onTaxes,RulesandPer diemTaxes(now we will be testingAmountPickercomponent)+Add rate -> ValueAmountPickerworks properly by pressing it and adding the valueRules(now we will be testingPercentageFormcomponent)Auto-approve compliant reportsPercentageFormworks properly by changing the value ofRandom report auditPer diem(now we will be testingTextLinkcomponent)TextLinkworks properly by pressing it and checking if theExpensifyHelpsite opensPer Diem Rate Templates+floating button in the left bottom corner (now we will be testingTimeModalPickercomponent)Create expense -> Per diemand choose any per diem rateTimeModalPickerworks properly by changingStart timeorEnd timeof the per diem expenseReports -> FiltersAmount(now we will be testingAmountWithoutCurrencyInputcomponent)Equal to,Greater than,Less thanAmountWithoutCurrencyInputworks properly by changing the value of one of the above and checking if it filtered the results properlywithViewportOffsetTopwhich is used to open some of the popovers properly -> in this case theEmojiPicker)withViewportOffsetTopworks properly by pressing theEmojiPickerand choosing some emoji - check if it's properly added to the message composerThis one needs to be done on staging
56. Go to URL https://staging.new.expensify.com/
57. Login with any account
58. Verify that
FormScrollViewworks properly by opening the enable payments page http://staging.new.expensify.com/enable-payments - if it loads correctly and you are able to go further with the flow then it worksThis ones need to be done on native (the infinite loading error needs to be checked on ios)
59. Go to the
Photos/Galleryon the phone and share some image withExpensify60. Verify that
ShareTabParticipantsSelectorworks properly by sharing the image with other user/workspace61. Now go to any 1:1 chat and add a few images in the chat
62. Press on the image so that the
AttachmentCarouselopens up63. Verify that the
Pagerworks correctly by traversing through the attachments64. Go to any 1:1 chat and add a few messages
65. Verify that
ActionSheetAwareScrollViewworks properly by editing one of the messages66. While still in 1:1 chat click the + Icon
67. Select Add attachment
68. Select either "Take photo", "Choose from gallery", "Choose file"
69. Verify that the infinite loading doesn't occur (we want to avoid this situation -> #72092 (comment))
Offline tests
Same as Tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Steps from 59 to 65:
59to65.mp4
Steps from 66 to 69:
66to65.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Steps from 1 to 18:
1to18.mp4
Steps from 19 to 24:
19to24.mp4
Steps from 25 to 31:
25to31.mp4
Steps from 32 to 49:
32to49.mp4
Steps from 50 to 55:
50to55.mp4
This one has to be done on staging
Steps from 56 to 58:
56to58.mp4
MacOS: Desktop