-
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
prevent FAB menu and other page showing simultaneously #12655
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1ddcbbe
prevent FAB menu and other page showing simultaneously
aimane-chnaif e6c6305
small refactor on SidebarLinks, BaseSidebarScreen
aimane-chnaif fc227dd
fix typo
aimane-chnaif 2e614d9
Merge branch 'main' of https://github.com/aimane-chnaif/Expensify int…
aimane-chnaif b8f6762
fix typo
aimane-chnaif f20e5aa
add comments for didScreenBecomeInactive, isScreenInactive functions
aimane-chnaif File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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 = { | ||
|
@@ -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 = { | ||
|
@@ -62,29 +63,79 @@ 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 status changed from active to inactive. | ||
* Used to close already opened FAB menu when open any other pages (i.e. Press Command + K on web). | ||
* | ||
* @param {Object} prevProps | ||
* @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. | ||
* Used to prevent FAB menu showing after opening any other pages. | ||
* | ||
* @return {Boolean} | ||
*/ | ||
isScreenInactive() { | ||
// When drawer is closed and Report page is open | ||
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
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. Do we need this? 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. yes to avoid unnecessary call when press shortcut key without opening popup menu |
||
this.props.onHideCreateMenu(); | ||
this.setState({ | ||
isCreateMenuActive: false, | ||
|
@@ -113,9 +164,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 | ||
|
@@ -193,4 +244,7 @@ class BaseSidebarScreen extends Component { | |
BaseSidebarScreen.propTypes = propTypes; | ||
BaseSidebarScreen.defaultProps = defaultProps; | ||
|
||
export default withDrawerState(BaseSidebarScreen); | ||
export default compose( | ||
withDrawerState, | ||
withNavigationFocus, | ||
)(BaseSidebarScreen); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What is difference between didBecomeInactive and this funciton. Why can't we use didBecomeInactive after combining those?
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.
didBecomeInactive
is just after screen is blurred. called only insidecomponentDidUpdate
. 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.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.
Ok
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.
Add JSDocs for both functions with details.
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.
@aimane-chnaif Can you please do this?
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.
@parasharrajat I think it's done in latest commit. What's missing?
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.
Comments for functions. There is no details about these functions and how they are used.
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.
please check now