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.

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

prevent FAB menu and other page showing simultaneously #12655

Merged
merged 6 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
45 changes: 37 additions & 8 deletions src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ const propTypes = {
/** Toggles the navigation menu open and closed */
onLinkClick: PropTypes.func.isRequired,

/** Navigates to settings and hides sidebar */
onAvatarClick: PropTypes.func.isRequired,

/** Safe area insets required for mobile devices margins */
insets: safeAreaInsetPropTypes.isRequired,

Expand Down Expand Up @@ -83,10 +80,45 @@ const defaultProps = {
};

class SidebarLinks extends React.Component {
constructor(props) {
super(props);

this.showSearchPage = this.showSearchPage.bind(this);
this.showSettingsPage = this.showSettingsPage.bind(this);
this.showReportPage = this.showReportPage.bind(this);
}

showSearchPage() {
if (this.props.isCreateMenuOpen) {
// Prevent opening Search page when click Search icon quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.SEARCH);
}

showSettingsPage() {
if (this.props.isCreateMenuOpen) {
// Prevent opening Settings page when click profile avatar quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.SETTINGS);
}

/**
* Show Report page with selected report id
*
* @param {Object} option
* @param {String} option.reportID
*/
showReportPage(option) {
if (this.props.isCreateMenuOpen) {
// Prevent opening Report page when click LHN row quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
this.props.onLinkClick();
}

render() {
// Wait until the personalDetails are actually loaded before displaying the LHN
if (_.isEmpty(this.props.personalDetails)) {
Expand Down Expand Up @@ -129,7 +161,7 @@ class SidebarLinks extends React.Component {
<TouchableOpacity
accessibilityLabel={this.props.translate('sidebarScreen.buttonMySettings')}
accessibilityRole="button"
onPress={this.props.onAvatarClick}
onPress={this.showSettingsPage}
>
<AvatarWithIndicator
source={this.props.currentUserPersonalDetails.avatar}
Expand All @@ -147,10 +179,7 @@ class SidebarLinks extends React.Component {
focusedIndex={_.findIndex(optionListItems, (
option => option.toString() === this.props.reportIDFromRoute
))}
onSelectRow={(option) => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
this.props.onLinkClick();
}}
onSelectRow={this.showReportPage}
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
onLayout={App.setSidebarLoaded}
Expand Down
72 changes: 62 additions & 10 deletions src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import SidebarLinks from '../SidebarLinks';
import PopoverMenu from '../../../../components/PopoverMenu';
import FloatingActionButton from '../../../../components/FloatingActionButton';
import ScreenWrapper from '../../../../components/ScreenWrapper';
import compose from '../../../../libs/compose';
import Navigation from '../../../../libs/Navigation/Navigation';
import ROUTES from '../../../../ROUTES';
import Timing from '../../../../libs/actions/Timing';
Expand All @@ -19,6 +20,7 @@ import Performance from '../../../../libs/Performance';
import * as Welcome from '../../../../libs/actions/Welcome';
import {sidebarPropTypes, sidebarDefaultProps} from './sidebarPropTypes';
import withDrawerState from '../../../../components/withDrawerState';
import withNavigationFocus from '../../../../components/withNavigationFocus';
import KeyboardShortcutsModal from '../../../../components/KeyboardShortcutsModal';

const propTypes = {
Expand Down Expand Up @@ -46,7 +48,6 @@ class BaseSidebarScreen extends Component {

this.hideCreateMenu = this.hideCreateMenu.bind(this);
this.startTimer = this.startTimer.bind(this);
this.navigateToSettings = this.navigateToSettings.bind(this);
this.showCreateMenu = this.showCreateMenu.bind(this);

this.state = {
Expand All @@ -62,29 +63,77 @@ class BaseSidebarScreen extends Component {
Welcome.show({routes, showCreateMenu: this.showCreateMenu});
}

componentDidUpdate(prevProps) {
if (!this.didScreenBecomeInactive(prevProps)) {
return;
}

// Hide menu manually when other pages are opened using shortcut key
this.hideCreateMenu();
}

/**
* Check if LHN became inactive from active status
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Check if LHN became inactive from active status
* Check if LHN status changed from active to inactive

*
* @param {Object} prevProps
* @return {boolean}
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the style guide

Suggested change
* @return {boolean}
* @return {Boolean}

*/
didScreenBecomeInactive(prevProps) {
// When the Drawer gets closed and ReportScreen is shown
if (!this.props.isDrawerOpen && prevProps.isDrawerOpen) {
return true;
}

// When any other page is opened over LHN
if (!this.props.isFocused && prevProps.isFocused) {
return true;
}

return false;
}

/**
* Check if LHN is inactive
*
* @return {boolean}
*/
isScreenInactive() {
// When drawer is closed and Report page is open
Copy link
Member

@parasharrajat parasharrajat Nov 15, 2022

Choose a reason for hiding this comment

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

What is difference between didBecomeInactive and this funciton. Why can't we use didBecomeInactive after combining those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didBecomeInactive is just after screen is blurred. called only inside componentDidUpdate. so this is the event and called automatically when any props or state updated.
isInactive is when screen is already unfocused. so this is not the event but is manually called.
Hiding menu should be triggered at the event of screen blur. isInactive cannot be used here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok

Copy link
Member

Choose a reason for hiding this comment

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

Add JSDocs for both functions with details.

Copy link
Member

Choose a reason for hiding this comment

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

@aimane-chnaif Can you please do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat I think it's done in latest commit. What's missing?

Copy link
Member

Choose a reason for hiding this comment

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

Comments for functions. There is no details about these functions and how they are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check now

if (this.props.isSmallScreenWidth && !this.props.isDrawerOpen) {
return true;
}

// When any other page is open
if (!this.props.isFocused) {
return true;
}

return false;
}

/**
* Method called when we click the floating action button
*/
showCreateMenu() {
if (this.isScreenInactive()) {
// Prevent showing menu when click FAB icon quickly after opening other pages
return;
}
this.setState({
isCreateMenuActive: true,
});
this.props.onShowCreateMenu();
}

/**
* Method called when avatar is clicked
*/
navigateToSettings() {
Navigation.navigate(ROUTES.SETTINGS);
}

/**
* Method called either when:
* Pressing the floating action button to open the CreateMenu modal
* Selecting an item on CreateMenu or closing it by clicking outside of the modal component
*/
hideCreateMenu() {
if (!this.state.isCreateMenuActive) {
return;
}
Comment on lines +136 to +138
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes to avoid unnecessary call when press shortcut key without opening popup menu

this.props.onHideCreateMenu();
this.setState({
isCreateMenuActive: false,
Expand Down Expand Up @@ -113,9 +162,9 @@ class BaseSidebarScreen extends Component {
<SidebarLinks
onLinkClick={this.startTimer}
insets={insets}
onAvatarClick={this.navigateToSettings}
isSmallScreenWidth={this.props.isSmallScreenWidth}
isDrawerOpen={this.props.isDrawerOpen}
isCreateMenuOpen={this.state.isCreateMenuActive}
reportIDFromRoute={this.props.reportIDFromRoute}
/>
<FloatingActionButton
Expand Down Expand Up @@ -193,4 +242,7 @@ class BaseSidebarScreen extends Component {
BaseSidebarScreen.propTypes = propTypes;
BaseSidebarScreen.defaultProps = defaultProps;

export default withDrawerState(BaseSidebarScreen);
export default compose(
withDrawerState,
withNavigationFocus,
)(BaseSidebarScreen);
1 change: 0 additions & 1 deletion tests/utils/LHNTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ function getDefaultRenderedSidebarLinks(reportIDFromRoute = '') {
right: 0,
bottom: 0,
}}
onAvatarClick={() => {}}
isSmallScreenWidth={false}
reportIDFromRoute={reportIDFromRoute}
/>
Expand Down