-
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 crash when edit a message after coming back from a child report #21227
Changes from all commits
4ba3da9
4c0a531
217059f
fe19af2
c6fa8b6
cba1d02
90dee15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import {useContext} from 'react'; | ||
import ReportScreenContext from '../../pages/home/ReportScreenContext'; | ||
|
||
function useReportScrollManager() { | ||
const {flatListRef} = useContext(ReportScreenContext); | ||
|
||
/** | ||
* Scroll to the provided index. On non-native implementations we do not want to scroll when we are scrolling because | ||
* we are editing a comment. | ||
* | ||
* @param {Object} index | ||
* @param {Boolean} isEditing | ||
*/ | ||
const scrollToIndex = (index, isEditing) => { | ||
if (!flatListRef.current || isEditing) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToIndex(index); | ||
}; | ||
|
||
/** | ||
* Scroll to the bottom of the flatlist. | ||
*/ | ||
const scrollToBottom = () => { | ||
if (!flatListRef.current) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToOffset({animated: false, offset: 0}); | ||
}; | ||
|
||
return {ref: flatListRef, scrollToIndex, scrollToBottom}; | ||
} | ||
|
||
export default useReportScrollManager; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import {useContext} from 'react'; | ||
import ReportScreenContext from '../../pages/home/ReportScreenContext'; | ||
|
||
function useReportScrollManager() { | ||
const {flatListRef} = useContext(ReportScreenContext); | ||
|
||
/** | ||
* Scroll to the provided index. | ||
* | ||
* @param {Object} index | ||
*/ | ||
const scrollToIndex = (index) => { | ||
if (!flatListRef.current) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToIndex(index); | ||
}; | ||
|
||
/** | ||
* Scroll to the bottom of the flatlist. | ||
*/ | ||
const scrollToBottom = () => { | ||
if (!flatListRef.current) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToOffset({animated: false, offset: 0}); | ||
}; | ||
|
||
return {ref: flatListRef, scrollToIndex, scrollToBottom}; | ||
} | ||
|
||
export default useReportScrollManager; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,30 @@ | ||
import React from 'react'; | ||
|
||
// This ref is created using React.createRef here because this function is used by a component that doesn't have access | ||
// to the original ref. | ||
const flatListRef = React.createRef(); | ||
|
||
/** | ||
* Scroll to the provided index. On non-native implementations we do not want to scroll when we are scrolling because | ||
* we are editing a comment. | ||
* | ||
* @param {Object} ref | ||
* @param {Object} index | ||
* @param {Boolean} isEditing | ||
*/ | ||
function scrollToIndex(index, isEditing) { | ||
if (isEditing) { | ||
function scrollToIndex(ref, index, isEditing) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add safety checks here if (!ref.current) {
return;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for this but it could be simplified if (!ref.current || isEditing) { return; } Let's add safety checks for all other methods There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. Added in the hooks too |
||
if (!ref.current || isEditing) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToIndex(index); | ||
ref.current.scrollToIndex(index); | ||
} | ||
|
||
/** | ||
* Scroll to the bottom of the flatlist. | ||
* | ||
* @param {Object} ref | ||
*/ | ||
function scrollToBottom() { | ||
if (!flatListRef.current) { | ||
function scrollToBottom(ref) { | ||
if (!ref.current) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToOffset({animated: false, offset: 0}); | ||
ref.current.scrollToOffset({animated: false, offset: 0}); | ||
} | ||
|
||
export {flatListRef, scrollToIndex, scrollToBottom}; | ||
export {scrollToIndex, scrollToBottom}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,28 @@ | ||
import React from 'react'; | ||
|
||
// This ref is created using React.createRef here because this function is used by a component that doesn't have access | ||
// to the original ref. | ||
const flatListRef = React.createRef(); | ||
|
||
/** | ||
* Scroll to the provided index. | ||
* | ||
* @param {Object} ref | ||
* @param {Object} index | ||
*/ | ||
function scrollToIndex(index) { | ||
flatListRef.current.scrollToIndex(index); | ||
function scrollToIndex(ref, index) { | ||
if (!ref.current) { | ||
return; | ||
} | ||
|
||
ref.current.scrollToIndex(index); | ||
} | ||
|
||
/** | ||
* Scroll to the bottom of the flatlist. | ||
* | ||
* @param {Object} ref | ||
*/ | ||
function scrollToBottom() { | ||
if (!flatListRef.current) { | ||
function scrollToBottom(ref) { | ||
if (!ref.current) { | ||
return; | ||
} | ||
|
||
flatListRef.current.scrollToOffset({animated: false, offset: 0}); | ||
ref.current.scrollToOffset({animated: false, offset: 0}); | ||
} | ||
|
||
export {flatListRef, scrollToIndex, scrollToBottom}; | ||
export {scrollToIndex, scrollToBottom}; |
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.
As this is a functional component, why we don't just use
useReportScrollManager
? we should export refs from the hooks.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 one is reactionListRef, not the flat list one 😄
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.
But we use the same context ? inside the
useReportScrollManager
we should export both refs and use each ref accordingly .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 think that would be weird useReportScrollManager exporting reaction list ref. Reaction list does not belong to report scroll manager