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

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 all 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
32 changes: 8 additions & 24 deletions src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import React from 'react';
import {View} from 'react-native';
import _ from 'underscore';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import styles from '../../../styles/styles';
import * as StyleUtils from '../../../styles/StyleUtils';
import ONYXKEYS from '../../../ONYXKEYS';
Expand All @@ -30,7 +29,6 @@ import * as Session from '../../../libs/actions/Session';
import KeyboardShortcut from '../../../libs/KeyboardShortcut';
import onyxSubscribe from '../../../libs/onyxSubscribe';
import * as ReportActionContextMenu from '../report/ContextMenu/ReportActionContextMenu';
import withCurrentReportID from '../../../components/withCurrentReportID';
import SignInOrAvatarWithOptionalStatus from './SignInOrAvatarWithOptionalStatus';

const basePropTypes = {
Expand All @@ -53,22 +51,13 @@ const propTypes = {

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

/** The top most report id */
currentReportID: PropTypes.string,

/* Onyx Props */
report: PropTypes.shape({
/** reportID (only present when there is a matching report) */
reportID: PropTypes.string,
}),
isActiveReport: PropTypes.func.isRequired,

...withLocalizePropTypes,
};

const defaultProps = {
priorityMode: CONST.PRIORITY_MODE.DEFAULT,
currentReportID: '',
report: {},
};

class SidebarLinks extends React.PureComponent {
Expand Down Expand Up @@ -142,10 +131,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 Expand Up @@ -201,14 +194,5 @@ class SidebarLinks extends React.PureComponent {

SidebarLinks.propTypes = propTypes;
SidebarLinks.defaultProps = defaultProps;
export default compose(
withLocalize,
withWindowDimensions,
withCurrentReportID,
withOnyx({
report: {
key: ({currentReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${currentReportID}`,
},
}),
)(SidebarLinks);
export default compose(withLocalize, withWindowDimensions)(SidebarLinks);
export {basePropTypes};
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 Down Expand Up @@ -81,6 +81,10 @@ function SidebarLinksData({isFocused, allReportActions, betas, chatReports, curr
return reportIDsRef.current || [];
}, [allReportActions, betas, chatReports, currentReportID, policies, priorityMode, isLoading]);

const currentReportIDRef = useRef(currentReportID);
currentReportIDRef.current = currentReportID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this value assignment needed? currentReportID is set on the line above as the initial value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without that assignment, the ref would not be updated when currentReportID changes, I think.

const isActiveReport = useCallback((reportID) => currentReportIDRef.current === reportID, []);
Comment on lines +84 to +86
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we simplify the block like this?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have mentioned in my proposal:

For the callback, in order not to use currentReportID as a dependency of the callback, which leads to the callback re-render too often, we should save it in a ref instead.


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