prevent FAB menu and other page showing simultaneously #12655
Conversation
melvin-bot
bot
requested review from
Beamanator and
parasharrajat
and removed request for
a team
|
@Beamanator @parasharrajat One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Bump @parasharrajat anything we're waiting on here? |
|
Yup, for the weekend to be over. I will review this tomorrow. I already tested a few things but need to do the cross-platform testing. |
|
Aah makes sense, thanks @parasharrajat 😆 |
|
As a reminder, it's the last business day by when 50% bonus is applied on merge? |
|
Reviewing now. |
| @@ -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()} | |||
There was a problem hiding this comment.
Revert it please.
| onPress={() => this.showSearchPage()} | |
| onPress={this.showSearchPage} |
There was a problem hiding this comment.
I did this because this. is used in showSearchPage.
Or we can bind this function. What do you prefer?
| Navigation.navigate(ROUTES.SEARCH); | ||
| } | ||
|
|
||
| showReportPage(reportID) { |
There was a problem hiding this comment.
Missing jsDoc.
Only Param needs to be defined.
| return false; | ||
| } | ||
|
|
||
| isInactive() { |
There was a problem hiding this comment.
is Inactive what? Can we think of a better name? Functions are named on what they do not where they are called.
There was a problem hiding this comment.
Is this component (BaseSidebarScreen) inactive? Meaning screen is blurred because of other pages open
Maybe better to name isScreenInactive?
There was a problem hiding this comment.
Agreed, what about isSidebarScreenHidden?
There was a problem hiding this comment.
Screen is actually not hidden on web. just blurred by another modal
| this.hideCreateMenu(); | ||
| } | ||
|
|
||
| didBecomeInactive(prevProps) { |
There was a problem hiding this comment.
Same for it. A better name is needed. Functions are named on what they do not where they are called.
There was a problem hiding this comment.
Maybe better to name didScreenBecomeInactive?
There was a problem hiding this comment.
@Beamanator thoughts? I am not good with name.. 🐈
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Opposite of Screen just became inactive will be combination of
- Screen just became active
- Screen keeps active
- Screen keeps inactive
| if (!this.state.isCreateMenuActive) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
yes to avoid unnecessary call when press shortcut key without opening popup menu
| @@ -193,4 +243,4 @@ class BaseSidebarScreen extends Component { | |||
| BaseSidebarScreen.propTypes = propTypes; | |||
| BaseSidebarScreen.defaultProps = defaultProps; | |||
|
|
|||
| export default withDrawerState(BaseSidebarScreen); | |||
| export default withDrawerState(withNavigationFocus(BaseSidebarScreen)); | |||
| @@ -116,6 +165,7 @@ class BaseSidebarScreen extends Component { | |||
| onAvatarClick={this.navigateToSettings} | |||
| isSmallScreenWidth={this.props.isSmallScreenWidth} | |||
| isDrawerOpen={this.props.isDrawerOpen} | |||
| isMenuOpen={this.state.isCreateMenuActive} | |||
There was a problem hiding this comment.
| isMenuOpen={this.state.isCreateMenuActive} | |
| isCreateMenuOpen={this.state.isCreateMenuActive} |
| navigateToSettings() { | ||
| if (this.state.isCreateMenuActive) { | ||
| // Prevent opening Settings page when click profile avatar quickly after clicking FAB icon | ||
| return; | ||
| } | ||
| Navigation.navigate(ROUTES.SETTINGS); | ||
| } |
There was a problem hiding this comment.
Is there a possibility that we can move it inside the sidebarLinks component.
There was a problem hiding this comment.
yes, similar to Search page
| } | ||
|
|
||
| didBecomeInactive(prevProps) { | ||
| // When Report page is just opened |
There was a problem hiding this comment.
| // When Report page is just opened | |
| // When the Drawer gets closed and ReportScreen is shown |
| return true; | ||
| } | ||
|
|
||
| // When other page is just opened |
There was a problem hiding this comment.
| // When other page is just opened | |
| // When other page is opened over LHN. |
| } | ||
|
|
||
| isInactive() { | ||
| // When drawer is closed and Report page is open |
There was a problem hiding this comment.
What is difference between didBecomeInactive and this funciton. Why can't we use didBecomeInactive after combining those?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Add JSDocs for both functions with details.
There was a problem hiding this comment.
@aimane-chnaif Can you please do this?
There was a problem hiding this comment.
@parasharrajat I think it's done in latest commit. What's missing?
There was a problem hiding this comment.
Comments for functions. There is no details about these functions and how they are used.
There was a problem hiding this comment.
please check now
| return true; | ||
| } | ||
|
|
||
| // When other page is open |
There was a problem hiding this comment.
This returns true if any other page is open? Or a specific page? Let's be a bit clearer here
There was a problem hiding this comment.
correct. any other page is open except Report (this is handled with isDrawerOpen)
There was a problem hiding this comment.
Cool so how about this
| // When other page is open | |
| // When any other page is open |
| return false; | ||
| } | ||
|
|
||
| isInactive() { |
There was a problem hiding this comment.
Agreed, what about isSidebarScreenHidden?
| this.hideCreateMenu(); | ||
| } | ||
|
|
||
| didBecomeInactive(prevProps) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Overall i like the new function names, what do you think @parasharrajat?
| } | ||
|
|
||
| /** | ||
| * Check if LHN became inactive from active status |
There was a problem hiding this comment.
| * Check if LHN became inactive from active status | |
| * Check if LHN status changed from active to inactive |
| * Check if LHN became inactive from active status | ||
| * | ||
| * @param {Object} prevProps | ||
| * @return {boolean} |
There was a problem hiding this comment.
Following the style guide
| * @return {boolean} | |
| * @return {Boolean} |
|
PR is now settled for new review |
|
Testing. |
|
Waiting on #12655 |
Screenshots🔲 iOS / nativescreen-2022-11-16_03.00.25.mp4🔲 iOS / Safariscreen-2022-11-16_02.26.09.mp4🔲 MacOS / Desktop
🔲 MacOS / Chromescreen-2022-11-16_02.14.06.mp4🔲 Android / Chromescreen-2022-11-16_02.13.10.mp4🔲 Android / nativescreen-2022-11-16_03.06.16.mp4 |
There was a problem hiding this comment.
LGTM.
cc: @Beamanator
PR Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by adding the
Waiting for Copylabel for a copy review on the original GH to get the correct copy. - I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
🎀 👀 🎀 C+ reviewed
|
@parasharrajat are you sure the checklist you just used was latest? |
Reviewer Checklist
Screenshots/VideosSee above |
|
Updated |
|
@Beamanator looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
Both checklists were definitely passing 🙃 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @Beamanator in version: 1.2.29-0 🚀
|
|
🚀 Deployed to production by @AndrewGable in version: 1.2.29-7 🚀
|
Details
Result: other page opens. do nothing on FAB icon click
Result (native): FAB menu opens. don't open any other pages
Result (mWeb): 2 scenarios in mWeb since web is super fast in state change:
Result: FAB menu is closed while opening any other page
Fixed Issues
$ #11229
PROPOSAL: #11229 (comment), #11229 (comment)
Tests
Login with any account
Scenario 1:
Scenario 2:
Scenario 3:
Scenario 4:
Scenario 5:
NOTE: Scenario 1-4 is for mobile, Scenario 5 is for web/desktop
QA Steps
Scenario 1:
Scenario 2:
Scenario 3:
Scenario 4:
Scenario 5:
NOTE: Scenario 1-4 is for mobile, Scenario 5 is for web/desktop
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
web.mov
Mobile Web - Chrome
mchrome.mp4
Mobile Web - Safari
msafari.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mp4