[No QA] Fix scroll issue #2607
[No QA] Fix scroll issue #2607
Conversation
|
cc @Julesssss @Jag96 |
| @@ -80,6 +85,11 @@ const defaultProps = { | |||
| }; | |||
|
|
|||
| class IOUConfirmationList extends Component { | |||
| constructor(props) { | |||
| super(props); | |||
| this.minimumBottomOffset = 240; | |||
There was a problem hiding this comment.
Does 240 represent the size of the button area? I think this will change in the future, so would we need to make this dynamic in that case?
Also, I don't think it needs to be attached to IOUConfirmationList. Could you make this a constant please?
There was a problem hiding this comment.
It was based on hit and trial to accommodate the button as well as the comment box. Will include it in CONST.js then! :)
There was a problem hiding this comment.
But, isn't it more appropriate in IOUConfirmationList itself, considering it includes both the Button and Comment Box, which is only present in IOUConfirmationList right now? @Julesssss
There was a problem hiding this comment.
I mean more that it doesn't need to be a class prop. A const within this file would be fine too I think.
There was a problem hiding this comment.
@Julesssss Ahh makes sense! Made the change :)
There was a problem hiding this comment.
@barun1997 can you please add manual tests so that the PR reviewers and QA can verify the fix? For example, it isn't clear how to get to the split screen
|
@Jag96 It's there now :) |
|
I've added a note for QA Steps, as it's not yet possible for QA to launch the IOU flow for non-web platforms. I'll do this testing myself. |
|
@Julesssss I'll look for a solution. Not sure why it went below like that, is it on the Mobile web? |
|
@Julesssss I just found out it's only that when the user dynamically changes screen height. If you reload on the same height, the screen adapts. Will this be a blocker? |
|
Here's a video to explain it better: Screen.Recording.2021-04-29.at.20.28.31.mov |
Oh, that's a good point. I guess it probably shouldn't be a blocker in that case, but ideally, it would adapt dynamically after changing the screen height. |
Sounds great! I did look into the solutions, haven’t found an elegant solution yet. Majority of the solutions use a listener of some sort to see if the dimensions changed. But since dimensions changing isn’t something that would normally happen with a user, I feel it’d be too over-engineered. |
There was a problem hiding this comment.
This LGTM, I agree that the dimension resizing would be nice but isn't a blocker in this case. Will defer to @Julesssss for the final say on this.
| listContainerStyles={[{ | ||
| // Give max height to the list container so that it does not extend | ||
| // beyond the comment view as well as button | ||
| maxHeight: Dimensions.get('window').height - MINIMUM_BOTTOM_OFFSET |
There was a problem hiding this comment.
Any reason to use Dimensions.get here instead of updating this component to use withWindowDimensions?
There was a problem hiding this comment.
Didn't realise we had withWindowDimensions. Made the change now!
|
Ideally, we would make the whole page scroll. But that wasn't in the original requirements, so this can be done at a later date. |
|
🚀 Deployed to staging in version: 1.0.34-6🚀
|
|
I wouldn't have known how to properly review this PR so thanks for not blocking on me. I went ahead and read the commits and the review comments to learn so I can be better prepared for the next PR |
<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
Details
Initially, I solved the problem of the input boxes not lining up using flexGrow0 but that prompted the list to not grow.
So, I worked on a solution to instead give the OptionsList a maxHeight, so that anything beyond that would be scrollable.
Fixed Issues
Fixes #1988Tests
QA Steps
Please ping @Julesssss to run the QA steps, as this is not yet possible for external QA
Tested On
Screenshots
Web
Mobile Web
iOS