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

Hide Keyboard shortcuts menu in Native Devices #6881

Merged
merged 2 commits into from
Dec 22, 2021

Conversation

akshayasalvi
Copy link
Contributor

Details

Fixed Issues

$ #6879
$ #6660

Tests

  1. Tested the menu visibility on all platforms.

QA Steps

  1. Launch the app
  2. Log in with any account
  3. Go to Setting > About
  4. You shouldn't see the View Keyboard Shortcuts menu on Mobile Web, iOS and Android App.'

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-shortcuts-menu

Mobile Web

mweb-shortcuts-menu

Desktop

desktop-shortcuts-menu

iOS

ios-shortcuts-menu

Android

android-shortcuts-menu

@akshayasalvi akshayasalvi requested a review from a team as a code owner December 22, 2021 20:00
@MelvinBot MelvinBot requested review from roryabraham and removed request for a team December 22, 2021 20:00
};

const AboutPage = (props) => {
const shouldShowKeyboardShortcutsMenu = !props.isSmallScreenWidth && [CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP].indexOf(getPlatform()) > -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, due to our code guidelines we cannot use getPlatform here – using it is basically equivalent to using Platform.select or Platform.OS, which is prohibited in our app (consult the README, section Cross Platform 99.9999%. We should make this more explicit in our javascript style guide.

Anyways, the way you should implement this is:

  1. Update the module structure as follows:

    src/pages/settings/AboutPage/
    ----| index.js   // Move all the code from the current AboutPage.js to here
    ----| platformSpecificMenuItems
    --------| index.js    // Export an array that contains the keyboard shortcuts menu item on wide screens or an empty array on narrow screens (mobile web)
    --------| index.native.js // Export an empty array
    
  2. In the new AboutPage/index.js, import the platformSpecificMenuItems and add them to the array of menu items before rendering.

I know that this has the same result as what you've done here, but we've standardized on a code style that uses platform-specific file extensions instead of inline platform checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I am aware of the index.native.js, etc. but wasn't aware that getPlatform cannot be used. I'll update the PR

@parasharrajat
Copy link
Member

Sorry for the inconvenience but I think we should hold this PR as a new discussion is popped up to enable keyboard shortcuts on native. https://expensify.slack.com/archives/C01GTK53T8Q/p1640208283479700

we may have to rethink about hiding the link on native.

@akshayasalvi
Copy link
Contributor Author

Nevertheless I am done with the changes @roryabraham suggested.

@roryabraham
Copy link
Contributor

@jasperhuangg Since pull request fixes a deploy-blocking regression, I am going to test this, and if it works then I'll merge it and CP to staging. We can treat #6883 as a separate issue.

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.23-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Jan 4, 2022

🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants