migrated ScrollViewWithContext to a functional component #23879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@s77rt I was wondering what screenshots should I add in the PR?
i don't think it'll be a good idea to find all 38+ places and add 38+ screenshots |
|
@Parshvi16 You should test for all or as many as possible cases (depends) but for the screenshots you can just add for one case |
s77rt
suggested changes
Jul 30, 2023
| } | ||
| } | ||
| ScrollViewWithContext.propTypes = propTypes; | ||
| const ScrollViewWithContext = React.forwardRef( |
There was a problem hiding this comment.
Write this as function ScrollViewWithContext and we don't need React.forwardRef we already forward the ref in the exported function
| const ScrollViewWithContext = React.forwardRef( | ||
| ({ onScroll, scrollEventThrottle, children, innerRef, ...restProps }, ref) => { | ||
| const [contentOffsetY, setContentOffsetY] = useState(0); | ||
| const scrollViewRef = innerRef || ref || defaultScrollViewRef; |
There was a problem hiding this comment.
For the fallback ref we should use useRef
There was a problem hiding this comment.
done. can't add directly useRef(null) in line 15 in place of defaultScrollViewRef because it throughs error that useRef cannot be called conditionally.
s77rt
suggested changes
Jul 30, 2023
| )); | ||
|
|
||
| export {ScrollContext}; | ||
| export { ScrollContext }; |
There was a problem hiding this comment.
Missing new line. Please run prettier (npm run prettier)
| } | ||
| ); | ||
|
|
||
| ScrollViewWithContext.propTypes = propTypes |
|
@s77rt I've added the screenshots and have addressed all the suggestions, so the PR is ready from my side. |
s77rt
suggested changes
Jul 31, 2023
| function ScrollViewWithContext({onScroll, scrollEventThrottle, children, innerRef, ...restProps}, ref) { | ||
| const [contentOffsetY, setContentOffsetY] = useState(0); | ||
| const defaultScrollViewRef = useRef(); | ||
| const scrollViewRef = innerRef || ref || defaultScrollViewRef; |
There was a problem hiding this comment.
Why do we still have innerRef and ref? Shouldn't it be just innerRef?
There was a problem hiding this comment.
removed extra ref
s77rt
suggested changes
Jul 31, 2023
Comment on lines
11
to
17
| /* | ||
| * <ScrollViewWithContext /> is a wrapper around <ScrollView /> that provides a ref to the <ScrollView />. | ||
| * <ScrollViewWithContext /> can be used as a direct replacement for <ScrollView /> | ||
| * if it contains one or more <Picker /> / <RNPickerSelect /> components. | ||
| * Using this wrapper will automatically handle scrolling to the picker's <TextInput /> | ||
| * when the picker modal is opened | ||
| */ |
There was a problem hiding this comment.
Can you please add the comment back?
Comment on lines
25
to
27
| ref={scrollViewRef} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...restProps} |
There was a problem hiding this comment.
Suggested change
| ref={scrollViewRef} | |
| // eslint-disable-next-line react/jsx-props-no-spreading | |
| {...restProps} | |
| // eslint-disable-next-line react/jsx-props-no-spreading | |
| {...restProps} | |
| ref={scrollViewRef} |
NAB but in general it's better to spread the props at first so they don't overwrite our custom passed props
|
@s77rt again, added all the suggested changes |
|
@Parshvi16 Please add tests steps and for the screenshot you should attach a video/screenshot for something that was effected by this PR. Prerequisites: Have a workspace (with USD set as the default currency)
Kooha-2023-07-31-19-01-21.mp4 |
Reviewer Checklist
Screenshots/VideosWebweb.mp4Mobile Web - Chromemweb-chrome.mp4Mobile Web - Safarimweb-safari.mp4Desktopdesktop.mp4iOSios.mp4Androidandroid.mp4 |
|
@s77rt updated the Test steps and added the test videos in the PR. |
madmax330
approved these changes
Aug 1, 2023
|
@madmax330 before merging, please assign me to the issue. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
79 tasks
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.3.49-0 🚀
|
86 tasks
Closed
1 task
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.3.50-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.49-3 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.50-3 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Migrated ScrollViewWithContext.js to a functional component
Fixed Issues
$ #16200
PROPOSAL: #16200 (comment)
Tests
Prerequisites: Have a workspace (with USD set as the default currency)
Offline tests
Same as online tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Screen.Recording.2023-08-01.at.11.35.37.AM.mov
Mobile Web - Chrome
Screen.Recording.2023-08-01.at.11.50.12.AM.mov
Mobile Web - Safari
Screen.Recording.2023-08-01.at.11.40.13.AM.online-video-cutter.com.mp4
Desktop
Screen.Recording.2023-08-01.at.11.46.33.AM.online-video-cutter.com.mp4
iOS
Screen.Recording.2023-08-01.at.11.38.56.AM.mov
Android
257311646-01b41d33-c0eb-4f21-bad0-197eba53ec32.mp4