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 1 commit
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
20 changes: 15 additions & 5 deletions src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,22 @@ const defaultProps = {

class SidebarLinks extends React.Component {
showSearchPage() {
if (this.props.isMenuOpen) {
// Prevent opening Search page when click Search icon quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.SEARCH);
}

showReportPage(reportID) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing jsDoc.

Only Param needs to be defined.

if (this.props.isMenuOpen) {
// Prevent opening Report page when click LHN row quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.getReportRoute(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 @@ -121,7 +134,7 @@ class SidebarLinks extends React.Component {
accessibilityLabel={this.props.translate('sidebarScreen.buttonSearch')}
accessibilityRole="button"
style={[styles.flexRow, styles.ph5]}
onPress={this.showSearchPage}
onPress={() => this.showSearchPage()}
Copy link
Member

Choose a reason for hiding this comment

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

Revert it please.

Suggested change
onPress={() => this.showSearchPage()}
onPress={this.showSearchPage}

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 did this because this. is used in showSearchPage.
Or we can bind this function. What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

Bind it and use it.

>
<Icon src={Expensicons.MagnifyingGlass} />
</TouchableOpacity>
Expand All @@ -147,10 +160,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={option => this.showReportPage(option.reportID)}
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
onLayout={App.setSidebarLoaded}
Expand Down
52 changes: 51 additions & 1 deletion src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,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 @@ -62,10 +63,51 @@ class BaseSidebarScreen extends Component {
Welcome.show({routes, showCreateMenu: this.showCreateMenu});
}

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

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

didBecomeInactive(prevProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Same for it. A better name is needed. Functions are named on what they do not where they are called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe better to name didScreenBecomeInactive?

Copy link
Member

Choose a reason for hiding this comment

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

@Beamanator thoughts? I am not good with name.. 🐈

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I'd rather not negate a negative - we're checking if the outcome of this function is false (!this.didBecomeInactive(prevProps)) and the function name is about something being "inactive" - it's clearer to check if something is active (instead of not inactive)

So how about this name: checkSidebarScreenNewlyVisible? I know it's a long name but explains the purpose of the function, I believe

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 think to check if something is active cannot be used here.
We check if screen just became inactive from active status.
What is the best naming after inverting this?
Screen is active or Screen just became active cannot be opposite of Screen just became inactive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opposite of Screen just became inactive will be combination of

  • Screen just became active
  • Screen keeps active
  • Screen keeps inactive

// When Report page is just opened
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// When Report page is just opened
// When the Drawer gets closed and ReportScreen is shown

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

// When other page is just opened
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// When other page is just opened
// When other page is opened over LHN.

if (!this.props.isFocused && prevProps.isFocused) {
return true;
}

return false;
}

isInactive() {
Copy link
Member

Choose a reason for hiding this comment

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

is Inactive what? Can we think of a better name? Functions are named on what they do not where they are called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this component (BaseSidebarScreen) inactive? Meaning screen is blurred because of other pages open
Maybe better to name isScreenInactive?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, what about isSidebarScreenHidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen is actually not hidden on web. just blurred by another modal

// 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 other page is open
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns true if any other page is open? Or a specific page? Let's be a bit clearer here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. any other page is open except Report (this is handled with isDrawerOpen)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool so how about this

Suggested change
// When other page is open
// 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.isInactive()) {
// Prevent showing menu when click FAB icon quickly after opening other pages
return;
}
this.setState({
isCreateMenuActive: true,
});
Expand All @@ -76,6 +118,10 @@ class BaseSidebarScreen extends Component {
* Method called when avatar is clicked
*/
navigateToSettings() {
if (this.state.isCreateMenuActive) {
// Prevent opening Settings page when click profile avatar quickly after clicking FAB icon
return;
}
Navigation.navigate(ROUTES.SETTINGS);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a possibility that we can move it inside the sidebarLinks component.

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, similar to Search page


Expand All @@ -85,6 +131,9 @@ class BaseSidebarScreen extends Component {
* 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 @@ -116,6 +165,7 @@ class BaseSidebarScreen extends Component {
onAvatarClick={this.navigateToSettings}
isSmallScreenWidth={this.props.isSmallScreenWidth}
isDrawerOpen={this.props.isDrawerOpen}
isMenuOpen={this.state.isCreateMenuActive}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isMenuOpen={this.state.isCreateMenuActive}
isCreateMenuOpen={this.state.isCreateMenuActive}

reportIDFromRoute={this.props.reportIDFromRoute}
/>
<FloatingActionButton
Expand Down Expand Up @@ -193,4 +243,4 @@ class BaseSidebarScreen extends Component {
BaseSidebarScreen.propTypes = propTypes;
BaseSidebarScreen.defaultProps = defaultProps;

export default withDrawerState(BaseSidebarScreen);
export default withDrawerState(withNavigationFocus(BaseSidebarScreen));
Copy link
Member

Choose a reason for hiding this comment

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

Use compose instead.