Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Composer does not refocus when click the same chat #24360

Merged
merged 13 commits into from
Sep 6, 2023
Merged

Fix: Composer does not refocus when click the same chat #24360

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2927390
fix: composer does not refocus when click the same chat
tienifr Aug 10, 2023
adb010e
prettier
tienifr Aug 10, 2023
67e5169
add comment
tienifr Aug 10, 2023
fe9034b
remove console log
tienifr Aug 10, 2023
923d512
Merge branch 'main' of https://github.com/tienifr/App into fix/23676-…
tienifr Aug 11, 2023
bc0b9df
useRef instead of usePrevious
tienifr Aug 11, 2023
ac5f2dc
fix lint
tienifr Aug 11, 2023
cbbc747
Merge branch 'main' into fix/23676
tienifr Aug 12, 2023
cf5e3ca
move all currentReportID out of SidebarLinks
tienifr Aug 14, 2023
e8378ed
Merge branch 'main' into fix/23676
tienifr Aug 29, 2023
cb03fc8
fix lint
tienifr Aug 29, 2023
4a671f7
stale commit
tienifr Aug 29, 2023
d841942
fix lint
tienifr Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ const propTypes = {

priorityMode: PropTypes.oneOf(_.values(CONST.PRIORITY_MODE)),

isActiveReport: PropTypes.func.isRequired,

...withLocalizePropTypes,
};

Expand Down Expand Up @@ -150,10 +152,14 @@ class SidebarLinks extends React.PureComponent {
*/
showReportPage(option) {
// Prevent opening Report page when clicking LHN row quickly after clicking FAB icon
// or when clicking the active LHN row
// or when clicking the active LHN row on large screens
// or when continuously clicking different LHNs, only apply to small screen
// since getTopmostReportId always returns on other devices
if (this.props.isCreateMenuOpen || this.props.currentReportID === option.reportID || (this.props.isSmallScreenWidth && Navigation.getTopmostReportId())) {
if (
this.props.isCreateMenuOpen ||
(!this.props.isSmallScreenWidth && this.props.isActiveReport(option.reportID)) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On small screen devices, when we navigate from report screen back to LHN, the currentReportID is set to undefined. But as prevCurrentReportID is saved in a ref, we cannot update the ref to undefined which leads to prevCurrentReportID unchanged and thus cannot navigate back to that same report again.

Also, the expected behavior only works on large screen devices so I added this check.

(this.props.isSmallScreenWidth && Navigation.getTopmostReportId())
) {
return;
}
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
Expand Down
7 changes: 6 additions & 1 deletion src/pages/home/sidebar/SidebarLinksData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useMemo, useRef} from 'react';
import React, {useCallback, useMemo, useRef} from 'react';
import _ from 'underscore';
import {deepEqual} from 'fast-equals';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -15,6 +15,7 @@ import CONST from '../../../CONST';
import useLocalize from '../../../hooks/useLocalize';
import styles from '../../../styles/styles';
import withNavigationFocus from '../../../components/withNavigationFocus';
import usePrevious from '../../../hooks/usePrevious';

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LINT

const propTypes = {
...basePropTypes,
Expand Down Expand Up @@ -75,6 +76,9 @@ function SidebarLinksData({isFocused, allReportActions, betas, chatReports, curr
return reportIDs;
}, [allReportActions, betas, chatReports, currentReportID, policies, priorityMode]);

const prevCurrentReportID = usePrevious(currentReportID);
const isActiveReport = useCallback((reportID) => prevCurrentReportID === reportID, [prevCurrentReportID]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I know I proposed using usePrevious, but it's actually not a good idea here as it directly returns the value (ref.current). Plus its incorrect as it indeed returns the previous value.
This way the callback gets recreated for each currentReportID, which is something we want to avoid.

So instead you might want to consider the following code:

Suggested change
const prevCurrentReportID = usePrevious(currentReportID);
const isActiveReport = useCallback((reportID) => prevCurrentReportID === reportID, [prevCurrentReportID]);
const currentReportIDRef = useRef(currentReportID);
currentReportIDRef.current = currentReportID;
const isActiveReport = useCallback((reportID) => currentReportIDRef.current === reportID, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I agree.

const isLoading = _.isEmpty(chatReports) || isPersonalDetailsLoading;

return (
Expand All @@ -90,6 +94,7 @@ function SidebarLinksData({isFocused, allReportActions, betas, chatReports, curr
isSmallScreenWidth={isSmallScreenWidth}
priorityMode={priorityMode}
// Data props:
isActiveReport={isActiveReport}
isLoading={isLoading}
optionListItems={optionListItems}
/>
Expand Down
Loading