-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Jakubkalinski0/fix console errors related to forward ref batch7 #71896
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 #71896
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@jakubkalinski0 seems like we already have conflicts. Can you please update the author checklist too to make that action pass? thanks! |
|
@c3024 could you give it a test as well (confirm the diff from the previous PR) |
| import useActionSheetKeyboardSpacing from './useActionSheetKeyboardSpacing'; | ||
|
|
||
| const ActionSheetAwareScrollView = forwardRef<ScrollView, ActionSheetAwareScrollViewProps>(({style, children, ...props}, ref) => { | ||
| function ActionSheetAwareScrollView({style, children, isInvertedScrollView, 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.
isInvertedScrollView prop isn't used anywhere in the component. 🤔 Is it not required for iOS?
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.
Good catch! Something must have slipped when i was resolving merge conflicts with newest main yesterday. I will look into it
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.
Now everything is clear, those changes regarding isInvertedScrollView were reverted between the first PR of the batch 7 and this PR thus when I cherry-picked those commits I did it with those reverted changes in my code.
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.
You can see the PR that was reverted here: #70250
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 will remove those and if there is nothing more we are good to go
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 for the quick changes!
| type ActionSheetAwareScrollViewProps = PropsWithChildren<ScrollViewProps>; | ||
| type ActionSheetAwareScrollViewAnimationProps = { | ||
| /** Whether the child ScrollView is inverted */ | ||
| isInvertedScrollView?: boolean; |
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 change is not on main and this does not look related to the forwardRef migration?
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.
Same as above, I will look into it
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.
Same case as above, I wrote all the details in the conversation above
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
c3024
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.
LGTM!
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!
|
✋ 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.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀
|
@mountiny
Raised once again after it was reverted.
Original PR -> #70878
Revert -> #71669
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
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 messagesOffline 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
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