Use userAgent in order to know if device is mobile instead of canUseTouchScreen function #8634

mateusbra · 2022-04-13T20:10:58Z

Details

This PR will solves #7715.
At the moment we are using the API from navigator.virtualKeyboard to verify when we have it open or not. If we have it open, we consider that the device is mobile. The issue here is that virtualKeyboard isn't available for all browsers, and when we don't have it available we are looking at canUseTouchScreen() and if it returns true, we assume its mobile.
It gives us an issue, we can't assume the device is mobile just because it is touch-enabled, today we have an infinity of touch-enabled windows devices.

When we consider having a mobile device, the Return shortcut will work differently:

For mobile devices: It should create a new line.
For desktop devices: It should send the message.

For this reason we need to ensure which type of device user is running, so we can know and tell how it should work.

This PR will change the behavior of how we look that, assuming we have a mobile device when:

We have the virtual Keyboard open(need the API)
When we don't have the API, see the userAgent

Fixed Issues

$ #7715

Tests

Mobile devices:

Android,iOS,mWeb:

Login with your account at New Expensify.
Open a report.
See that the return button is creating a New line.

Desktop devices:

Windows,MacOS:

Login with your account at New Expensify.
Open a report.
See that the return button is sending the message.

important: Also test it with a touch-enabled desktop device, it should work the same as the desktop devices.

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Mobile devices:

Android,iOS,mWeb:

Login with your account at New Expensify.
Open a report.
See that the return button is creating a New line.

Desktop devices:

Windows,MacOS, Desktop:

Login with your account at New Expensify.
Open a report.
See that the return button is sending the message.

important: Also test it with a touch-enabled desktop device, it should work the same as the desktop devices.

Screenshots

Web

MacOS_Chrome.mov

Mobile Web

iOS_Safari.mov

Android_Chrome.mp4

Desktop

MacOS_Desktop.mov

iOS

iOS_Native.mov

Android

Android_Native.mp4

mateusbra · 2022-04-13T20:21:26Z

I'll provide the screenshots tests, but first I want to understand how you want me to do that?

IMO, isMobile should be moved to getBrowser lib.

getBrowser's intent is to return which browser user is navigating.
Our logic won't see browser at all, it will see the platform we are using by agentUser.
Why do you think we should add this logic within getBrowser's lib?

cc: @parasharrajat

parasharrajat · 2022-04-14T06:23:31Z

Why do you think we should add this logic within getBrowser's lib?

UserAgent check is eventually checking if the platform is a Mobile browser. UserAgent only works on browsers (right?). Now you are checking if the device is mobile. So basically it is testing a mobile browser.

we can change the name of the lib if needed.

mateusbra · 2022-04-15T21:39:01Z

Ok, if we are going to move IsMobile into getBrowser's lib how should we do that?
I suggest changing the folder name getBrowser into Browser and add our logic there:

Then we should import it as Browser.getBrowser and Browser.isMobile.

What do you think?

parasharrajat · 2022-04-15T21:55:51Z

Seems fine to me.

mateusbra · 2022-04-19T14:02:00Z

@parasharrajat do you think we should delete the index.native file and make the index.js -> index.web.js?

parasharrajat

I do not understand the hierarchy here. I thought you were adding the code to the same lib and renaming it to Browser.

mateusbra · 2022-04-19T16:02:37Z

Sorry, I think I missunderstood here.

Is that the hierarchy you're expecting to have?

Then within index.js we export both getBrowser and isMobile function, is that right?

parasharrajat · 2022-04-19T16:22:29Z

Yes.

mateusbra · 2022-04-19T17:16:28Z

@parasharrajat let me know what you think from the changes and what you think we should do with the index.js file:

App/src/libs/Browser/index.js

Line 1 in 44f23e1

export default () => '';

parasharrajat · 2022-04-19T18:18:43Z

src/libs/Browser/index.web.js

@@ -1,7 +1,7 @@
 import CONST from '../../CONST';

 /**
- * Fetch browser name from UA string
+ * Fetch browser name from UA string and returns the browser name


I think the old comment was sufficient.

parasharrajat

what you think we should do with the index.js file:

What are you suggesting?

parasharrajat · 2022-04-19T18:20:01Z

src/libs/Browser/index.web.js

+ * Allows us to identify whether the platform is mobile.
+ *
+ * https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent
+ *


Suggested change

* Allows us to identify whether the platform is mobile.

*

* https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent

*

* Whether the platform is a mobile browser.

* https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent

*

mateusbra · 2022-04-19T18:47:45Z

What are you suggesting?

We were using getBrowser and returning '' from its index.js when we are not accessing it from web.
Do you think we should cover thoses cenarios?
index.js:

function getBrowser() {
    return '';
}

function isMobile() {
   return '';
}
export {
    getBrowser,
    isMobile,
};

parasharrajat · 2022-04-19T18:49:30Z

Yeah. Otherwise, how will it work?

parasharrajat · 2022-04-19T18:49:50Z

src/libs/Browser/index.web.js

@@ -27,14 +27,21 @@ function getBrowser() {
    }

    match = match[1] ? match[1] : navigator.appName;
-    return match;
+    return match ? match.toLowerCase() : CONST.BROWSER.OTHER;


Why do we need this change?

because we were exporting it this way:

export default () => { const browser = getBrowser(); return browser ? browser.toLowerCase() : CONST.BROWSER.OTHER;

Now we are going to export more than one function, and it will keep the same behavior:

export { getBrowser, isMobile, };

…ents

mateusbra · 2022-04-20T13:10:09Z

@parasharrajat let me know if you think there's something else we should change or if we are ok to go.

parasharrajat · 2022-04-20T14:32:13Z

I will test it in few hours.

parasharrajat

LGTM.

cc: @chiragsalian

PR Reviewer Checklist

🎀 👀 🎀 C+ reviewed

chiragsalian

LGTM

PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section 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 Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- 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 checked that screenshots or videos are included 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
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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. toggleReport and not onIconClick).
- 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 tagging the marketing team 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
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 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 */
- Any functional components have the displayName property
- 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 this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- 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 Avatar is modified, I verified that Avatar is 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.

OSBotify · 2022-04-21T02:01:16Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-04-25T23:42:55Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.57-0 🚀

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

OSBotify · 2022-05-13T16:15:29Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

use userAgent in order to know if is mobile

440e8e7

mateusbra requested a review from a team as a code owner April 13, 2022 20:10

melvin-bot bot requested review from chiragsalian and parasharrajat and removed request for a team April 13, 2022 20:11

fix eslint problems

c690730

change comment

9b76679

move isMobile into Browser lib

f7c9f32

parasharrajat suggested changes Apr 19, 2022

View reviewed changes

getBrowser -> Browser

44f23e1

parasharrajat reviewed Apr 19, 2022

View reviewed changes

parasharrajat suggested changes Apr 19, 2022

View reviewed changes

created getBrowser and isMobile function within index.js, change comm…

cb8f291

…ents

mateusbra requested a review from parasharrajat April 20, 2022 13:09

parasharrajat approved these changes Apr 20, 2022

View reviewed changes

chiragsalian assigned mateusbra Apr 21, 2022

chiragsalian approved these changes Apr 21, 2022

View reviewed changes

chiragsalian merged commit b6bf89f into Expensify:main Apr 21, 2022

francoisl mentioned this pull request Apr 25, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

roryabraham mentioned this pull request Apr 27, 2022

[$4000] mWeb/safari - Menu - "Open app banner" at the top causes spacing issues at the bottom of the browser #8791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use userAgent in order to know if device is mobile instead of canUseTouchScreen function #8634

Use userAgent in order to know if device is mobile instead of canUseTouchScreen function #8634

mateusbra commented Apr 13, 2022 •

edited

Loading

mateusbra commented Apr 13, 2022 •

edited

Loading

parasharrajat commented Apr 14, 2022 •

edited

Loading

mateusbra commented Apr 15, 2022

parasharrajat commented Apr 15, 2022

mateusbra commented Apr 19, 2022

parasharrajat left a comment

mateusbra commented Apr 19, 2022

parasharrajat commented Apr 19, 2022

mateusbra commented Apr 19, 2022

parasharrajat Apr 19, 2022

parasharrajat left a comment

parasharrajat Apr 19, 2022

mateusbra commented Apr 19, 2022

parasharrajat commented Apr 19, 2022

parasharrajat Apr 19, 2022

mateusbra Apr 19, 2022

parasharrajat Apr 19, 2022

mateusbra commented Apr 20, 2022

parasharrajat commented Apr 20, 2022

parasharrajat left a comment •

edited

Loading

chiragsalian left a comment

OSBotify commented Apr 21, 2022

OSBotify commented Apr 25, 2022

OSBotify commented May 13, 2022

Use userAgent in order to know if device is mobile instead of canUseTouchScreen function #8634

Use userAgent in order to know if device is mobile instead of canUseTouchScreen function #8634

Conversation

mateusbra commented Apr 13, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mateusbra commented Apr 13, 2022 • edited Loading

parasharrajat commented Apr 14, 2022 • edited Loading

mateusbra commented Apr 15, 2022

parasharrajat commented Apr 15, 2022

mateusbra commented Apr 19, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

mateusbra commented Apr 19, 2022

parasharrajat commented Apr 19, 2022

mateusbra commented Apr 19, 2022

parasharrajat Apr 19, 2022

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat Apr 19, 2022

Choose a reason for hiding this comment

mateusbra commented Apr 19, 2022

parasharrajat commented Apr 19, 2022

parasharrajat Apr 19, 2022

Choose a reason for hiding this comment

mateusbra Apr 19, 2022

Choose a reason for hiding this comment

parasharrajat Apr 19, 2022

Choose a reason for hiding this comment

mateusbra commented Apr 20, 2022

parasharrajat commented Apr 20, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

chiragsalian left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

OSBotify commented Apr 21, 2022

OSBotify commented Apr 25, 2022

OSBotify commented May 13, 2022

mateusbra commented Apr 13, 2022 •

edited

Loading

mateusbra commented Apr 13, 2022 •

edited

Loading

parasharrajat commented Apr 14, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading