Refactor os & mobile util functions #7303
Conversation
cadeban
force-pushed
the
cadeban/utils2
branch
from
0d7b2b2 to
53a1010
Compare
cadeban
force-pushed
the
cadeban/utils2
branch
2 times, most recently
from
a8e298c to
5414838
Compare
cadeban
changed the title
- favor inline regex - abstract isMobile -> hasMobileUserAgent into new file
cadeban
force-pushed
the
cadeban/utils2
branch
from
5414838 to
8430fdc
Compare
| export function isMobile() { | ||
| return /android|blackberry|iemobile|ipad|iphone|ipod|opera mini|webos/i.test( | ||
| navigator.userAgent | ||
| ); | ||
| } |
There was a problem hiding this comment.
Very nice to see this gone, was quite confusing as we have a useIsMobile hook as well.
| export function defaultShouldShowIosIcon() { | ||
| return ( | ||
| /Mac|iPad|iPhone|iPod/.test(navigator.userAgent) || Capacitor.getPlatform() === 'ios' | ||
| ); | ||
| } |
There was a problem hiding this comment.
| export function defaultShouldShowIosIcon() { | |
| return ( | |
| /Mac|iPad|iPhone|iPod/.test(navigator.userAgent) || Capacitor.getPlatform() === 'ios' | |
| ); | |
| } | |
| export const defaultShouldShowIosIcon = () => | |
| /Mac|iPad|iPhone|iPod/.test(navigator.userAgent) || Capacitor.getPlatform() === 'ios' |
Tiny bit smaller and takes advantage of the bundlers ability to declare multiple constants at the same time.
This regex could also be extracted to a isApplePlatform function to be reused by the app store url function.
web/src/utils/helpers.ts
Outdated
| export function hasMobileUserAgent() { | ||
| return /android|blackberry|iemobile|ipad|iphone|ipod|opera mini|webos/i.test( | ||
| navigator.userAgent | ||
| ); | ||
| } |
There was a problem hiding this comment.
| export function hasMobileUserAgent() { | |
| return /android|blackberry|iemobile|ipad|iphone|ipod|opera mini|webos/i.test( | |
| navigator.userAgent | |
| ); | |
| } | |
| export const hasMobileUserAgent = () => /android|blackberry|iemobile|ipad|iphone|ipod|opera mini|webos/i.test( | |
| navigator.userAgent | |
| ); |
Just a tiny bit smaller, I also think we can ditch iemobile and blackbarry. As far as I know they don't support the app anyhow 😅
And is not webOS only used in TVs these days?
There was a problem hiding this comment.
Well, it's also being used for the windlayer and may need support on mobile web, so I'm a little shy about removing blackberry and iemobile just yet
Maybe people are browsing our web app on their tvs? 😅
| }, | ||
| { | ||
| userAgent: | ||
| 'Mozilla/5.0 (Linux; Android 6.0; Nexus 5 Build/MRA58N) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.77 Mobile Safari/537.36', |
There was a problem hiding this comment.
Should Mobile be a part of the regex test?
There was a problem hiding this comment.
That seems reasonable! 👍
|
@cadeban could you try and get this merged today? Before we get even more merge conflicts 😬 |
|
Will handle the merge conflicts later today. Some of the work that came out of the Avo sync jumped ahead in the queue |
There was a problem hiding this comment.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no suggestions.
Files not reviewed (4)
- web/src/features/panels/zone/ShareButton.tsx: Evaluated as low risk
- web/src/features/weather-layers/wind-layer/windy.ts: Evaluated as low risk
- web/src/translation/i18n.ts: Evaluated as low risk
- web/src/features/panels/zone/ShareButton.test.tsx: Evaluated as low risk
|
@cadeban can we try and get this merged or put it in draft/close it? It's been open for a while now 🙂 |
Description
AVO-552: Refactor os & mobile util functions because while we like spaghetti, we don't like it in our code. 🙅🍝
🎵 Cha-cha-cha-changes:
Capacitor.isNativePlatform()to determine if mobileisMobile->hasMobileUserAgentStill TODO:
Additional discussion:
Preview
n/a, visual & functionality should be the same
Double check
poetry run test_parser "zone_key"pnpx prettier@2 --write .andpoetry run formatin the top level directory to format my changes.