Ensure multiple waypoints are visible on smaller screens#26604
Ensure multiple waypoints are visible on smaller screens#26604ZhenjaHorbach wants to merge 5 commits into
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
@marcochavezf 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] |
|
@ZhenjaHorbach Can you make sure to remove the unverified commits? |
|
@ZhenjaHorbach Can you please force push just with verified commits? |
|
You also need to sign the CLA as a first time contributor, simple comment the sentence its asking you to post |
bc1c085 to
9fd1f42
Compare
|
@ZhenjaHorbach Amazing, can you please update the tests to be a nubered list? than also fill out the chekclist and include videos from all the platfroms @fedirjh Could you start your review here in parallel, please? thanks! |
|
@ZhenjaHorbach can you post the sentence without anything else in there |
|
@ZhenjaHorbach Lint is failing as well |
| } | ||
|
|
||
| scrollViewRef.current.scrollToEnd({animated: true}); | ||
| }, [haveWaypointsChanged]) |
There was a problem hiding this comment.
We have a lint error here.
@ZhenjaHorbach I have an alternative suggestion: instead of using a hook to scroll, I will comment on another line.
| @@ -170,6 +180,7 @@ function DistanceRequest({iou, iouType, report, transaction, mapboxAccessToken}) | |||
| onContentSizeChange={(width, height) => setScrollContentHeight(height)} | |||
There was a problem hiding this comment.
Instead of using a hook for scrolling, we can utilize this callback to scroll when the content size changes and the number of waypoints increases:
onContentSizeChange={(width, height) => {
if (scrollContentHeight < height && numberOfWaypoints > numberOfPreviousWaypoints){
scrollViewRef.current.scrollToEnd({animated: true});
}
setScrollContentHeight(height)
}}There was a problem hiding this comment.
Actually, we can just compare the height and remove the extra changes of numberOfPreviousWaypoints.
There was a problem hiding this comment.
I totally agree about remove useEffect.
Good idea
But what about the height
I think it's better to compare the number of stops, because for example when opening the stop details
The height will reset and scroll will occur, which we do not need
More precisely, I believe that the change in the number of stops is much more predictable than the height
There was a problem hiding this comment.
cc @ZhenjaHorbach In case you missed my last comment, what do you think of just comparing the height?
Edit: Just missed your comment, I agree with your point 👍🏼
Reviewer Checklist
Screenshots/VideosWebCleanShot.2023-09-03.at.16.04.27-converted.mp4Mobile Web - ChromeCleanShot.2023-09-03.at.16.41.29-converted.mp4CleanShot.2023-09-03.at.16.42.48.mp4Mobile Web - SafariCleanShot.2023-09-03.at.16.29.59.mp4CleanShot.2023-09-03.at.16.32.20.mp4DesktopCleanShot.2023-09-03.at.16.13.47.mp4iOSCleanShot.2023-09-03.at.16.27.21.mp4AndroidCleanShot.2023-09-03.at.16.57.02.mp4 |
|
Looks like commits are messed up a little. The easiest solution would be to squash them into one commit if you can't update the previous commits. 
|
|
@ZhenjaHorbach can you fix the commutes please? If you prefer to make s new pr lets do it |
I'm just thinking about it |
|
|
Closing in favour of #26610 |
Details
Fixed Issues
$ #26595
PROPOSAL: N/A
Tests
1.Open app on small screen (height = +- 400px )
2.Go to a workspace chat
3.Click the "+" button next to the text composer
4.Click "Request money"
5.Click "Distance"
6.Should show 2 stops
7.Click Add stope
8.Should be an animation of the transition of the list elements to the end
Offline 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-09-03.at.17.09.30.mov
Screen.Recording.2023-09-03.at.17.05.56.mov
Mobile Web - Chrome
Screen.Recording.2023-09-03.at.17.20.13.mov
Mobile Web - Safari
Screen.Recording.2023-09-03.at.17.27.41.mov
Desktop
Screen.Recording.2023-09-03.at.17.14.00.mov
iOS
Android