-
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
fix: use correct prop type in BaseInvertedFlatlist component #21668
Conversation
@abdulrahuman5196 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] |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-06-27.at.8.09.51.PM.1.mp4Mobile Web - Chromeaz_recorder_20230627_201207.mp4Mobile Web - SafariUntitled.1.mp4DesktopScreen.Recording.2023-06-27.at.8.20.57.PM.mp4iOSScreen.Recording.2023-06-27.at.8.17.32.PM.mp4Androidaz_recorder_20230627_202847.mp4 |
@allroundexperts Kindly mark offline tests - "Same as tests" since this issue shouldn't come offline as well. |
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.
Changes looks good and works well. Reviewers checklist is also complete.
All yours @aldo-expensify
🎀👀🎀
C+ Reviewed
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'm very unfamiliarized with forward refs, but isn't this change going to cause a type miss-match at other levels of the tree?
For example here:
App/src/components/InvertedFlatList/index.js
Lines 10 to 12 in 70bf90d
innerRef: PropTypes.shape({ | |
current: PropTypes.instanceOf(FlatList), | |
}).isRequired, |
That one is still expecting a FlatList
type.
Once we introduce typescript, isn't this going to be a problem?
@@ -18,7 +18,7 @@ const propTypes = { | |||
initialRowHeight: PropTypes.number.isRequired, | |||
|
|||
/** Passed via forwardRef so we can access the FlatList ref */ | |||
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(FlatList)})]).isRequired, | |||
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(React.Component)})]).isRequired, |
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.
Why we didn't use PropTypes.instanceOf(BaseFlatList)
in the end?
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.
It seems we are using 'React.Component' at many of the forwarded ref component. So it should be fine to replicate the same to have consistency. I am also fine to use to this common refPropTypes created recently https://github.com/Expensify/App/blob/main/src/components/refPropTypes.js#L3
In the approval comment I had mentioned the above #20591 (comment)
It seems at multiple places in the app we are using the same behaviour. Kindly do let me know if we want to change this behaviour.
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.
hmm, I think we should try to have our prop types as tight as possible. This reference is used here for example:
App/src/libs/ReportScrollManager/index.js
Line 19 in 70bf90d
flatListRef.current.scrollToIndex(index); |
If you start passing around the type Component
instead of BaseFlatList
, typescript won't be able to find scrollToIndex
in Component
.
I'm not sure if this complicates things to much, then another option could be to leave this type problems for when we migrate to typescript 🤷
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.
Passing Component
around when you know it is a FlatList
(or BaseFlatList
) feels to me like using PropTypes.any
or PropTypes.object
(without specifying the shape of the object)... it works... but then we are missing the purpose of types (type safety)
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.
@aldo-expensify I think the main solution proposed here should solve this issue. We should import and use Flatlist
directly from RN.
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 it works, I think its better to be more precise about the type, so using FlatList
(or BaseFlatList
if FlatList
is not possible) is better IMO
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.
@aldo-expensify then we can do the same way as done here from your example
App/src/components/InvertedFlatList/index.js
Lines 3 to 13 in 70bf90d
import {FlatList, StyleSheet} from 'react-native'; | |
import _ from 'underscore'; | |
import BaseInvertedFlatList from './BaseInvertedFlatList'; | |
import styles from '../../styles/styles'; | |
const propTypes = { | |
/** Passed via forwardRef so we can access the FlatList ref */ | |
innerRef: PropTypes.shape({ | |
current: PropTypes.instanceOf(FlatList), | |
}).isRequired, | |
FlatList is imported to mention it as type. I highly doubt the current
variable will be of type BaseFlatList
, it will be of type FlatList
as it was the original ref.
If we do this way when we go to typescript it won't change AFAIK.
It is not a complex change and this was also one of the proposals on the same from @allroundexperts
It just I didn't want to add extra import for type and went with a consistent solution existing in the app.
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.
OOPS. Seems there where same suggestion from @allroundexperts and I also commented the same thing. (Page didn't refresh for me 😮💨 )
Yes. it is possible as mentioned here #21668 (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.
Ok!, yes use FlatList
from react-native
if possible.
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.
Updated @aldo-expensify @abdulrahuman5196!
Ready for another round @abdulrahuman5196 |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-06-29.at.12.21.23.AM.1.mp4Mobile Web - Chromeaz_recorder_20230629_002910.mp4Mobile Web - SafariUntitled.1.mp4DesktopScreen.Recording.2023-06-29.at.12.33.29.AM.1.mp4iOSScreen.Recording.2023-06-29.at.12.31.19.AM.mp4Androidaz_recorder_20230629_003848.mp4 |
@allroundexperts can we add the title of the warning we are fixing now in the QA tests section? Just so that when they are testing they shouldn't misjudge other warnings as regression, since there are some other errors in chat, but all have their own reportings and issues. |
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.
The changes looks good and works well. Reviewers checklist is also complete.
All yours. @aldo-expensify
🎀👀🎀
C+ Reviewed
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.
Thank you!
✋ 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/aldo-expensify in version: 1.3.35-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.35-5 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.35-5 🚀
|
Details
This PR fixes the console warning which was caused by an incorrect ref type being used.
Fixed Issues
$ #20591
PROPOSAL: #20591 (comment)
Tests
New Chat
and then select any chat.Verify that no new warning displays in the console
Offline tests
Same as above
QA Steps
New Chat
and then select any chat.Verify that no new warning displays in the console
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Screen.Recording.2023-06-27.at.4.01.29.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-06-27.at.4.04.08.PM.mov
Mobile Web - Safari
Screen.Recording.2023-06-27.at.4.03.48.PM.mov
Desktop
Screen.Recording.2023-06-27.at.4.05.03.PM.mov
iOS
Screen.Recording.2023-06-27.at.3.55.39.PM.mov
Android
Screen.Recording.2023-06-27.at.3.55.09.PM.mov