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.
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
Bidirectional pagination #26166
Bidirectional pagination #26166
Changes from 63 commits
426707f
cfa1c1e
89fefbf
741a3b9
a82dc0f
2619fa8
42fe76a
3df7a19
66483bf
a480524
2f69a83
694c423
35076a4
fe22397
e6f23df
045b07d
a9243e3
99cead6
bd27f36
063ef90
9fb4792
84d1953
cfe420d
120eefa
484f596
bde5a21
821adb8
652967c
5ef510d
fae75dd
2d094f0
5109429
84cb84c
0849919
a4f6834
1829ade
063af42
7dfc82e
1387d21
a3e609d
35f1ad3
6839c22
297e8ec
6a863a4
c1201d5
d48bdc8
34ea6f8
bd08f5e
f0ae93f
244cff4
6dd8147
cfb4bee
9043f69
8ebb468
bf9c309
f046ce1
f45bc44
06aeba6
b9b966c
9b04dd6
d5c16b0
d185048
463f96c
fece482
6c6adc4
92becc1
45f2f46
d508082
c9f0620
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 caused a regression when we try to load more while the device is offline it will cause a loop setting the value of
isLoadingNewerReportActions
creating a flicker #30503There 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 use of
setTimeout
is discouraged and a workaround. Why can't we callscrollToOffset
directly?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.
When a new item is added, it will appear in a new frame. If there's no timeout, the scroll will automatically move to the last message before the new item is added. As a result, the new item might be positioned outside the viewport
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 don't think that's the root cause. As far as I can tell the scroll mis alignment is caused by the added prop
maintainVisibleContentPosition
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.
Yes, you're right.
Report.addComment
and scroll are called one after the other, but addComment hasn't finished when the scroll runs, so the new item isn’t there yet. Once the item is added,maintainVisibleContentPosition
keeps the viewport in place. Hence, the scroll happens before the item is added, so it's expected behaviorThere 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.
Indeed the scroll happens first. But I still don't think the use of
setTimeout
is something that we want. It goes against our code pattern. The scroll is managed by ReportActionsList where we scroll immediately after getting the new event.@roryabraham Do you think we should open a new issue for the scroll bug? or handle it here?
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.
Potential alternate solution:
Report.addComment
return the optimisticreportActionID
of the new comment synchronously (i.e: not returning a promise, just an ID)reportActions
prop for this screen, then perform the scroll.I think this would be inline with our code patterns and would essentially be a workaround way to do
Report.addComment().then(...)
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.
So if that alternate solution works, it's a better plan than keeping the
setTimeout
and creating a follow-up issue – just address the bug with a long-term solution in this PR.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.
@roryabraham I'll be OOO until Friday. I barely fixed the conflicts as the internet here is terrible. I can handle this starting next week, or maybe we can proceed with your first solution involving a separate PR
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.
@s77rt, what do you think? Can we handle this PR before I come back from vacation? It would unblock the rest of the guys.
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.
@perunt Going to retest and complete the checklist now. @roryabraham Will handle whether this can be merged