feat: firefox extension permission ui #3864
Conversation
sshanzel
requested review from
rebelchris,
omBratteng,
ilasw,
capJavert,
nensidosari and
AmarTrebinjac
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
packages/shared/src/lib/func.ts
Outdated
| export const checkIsFirefox = (): boolean => | ||
| typeof globalThis?.navigator !== 'undefined' && | ||
| /firefox/i.test(globalThis?.navigator.userAgent); | ||
|
|
There was a problem hiding this comment.
Should we maybe add "firefox" to the UserAgent enum and use checkIsBrowser instead?
There was a problem hiding this comment.
Yeah, I thought of it at first but decided to use the one specifically for Firefox. Maybe yeah it is better to keep them on the same function by lowercasing the values from the userAgent.
| You can{' '} | ||
| <button type="button" className="text-text-link" onClick={onGoBack}> | ||
| go back | ||
| </button>{' '} |
There was a problem hiding this comment.
Are these intentional or just the linter? if you wrap "you can" and "and accept the required permissions." in spans, then just having a regular space between the elements will separate the words naturally. But it should also work to just have a regular space without any wrapping or {' '}
There was a problem hiding this comment.
Yeah, intentional by the linter :lolsob: , they get joined together without it. Not normally a fan adding a wrap without any additional attributes, but I'm open to it 👀
| @@ -106,6 +117,7 @@ function InternalApp(): ReactElement { | |||
| const isPageReady = | |||
| (growthbook?.ready && router?.isReady && isAuthReady) || isTesting; | |||
| const shouldRedirectOnboarding = !user && isPageReady && !isTesting; | |||
| const isFirefox = checkIsBrowser(UserAgent.Firefox); | |||
There was a problem hiding this comment.
Why don't you use the existing one?
isFirefoxExtension?
I think in this new flow we don't auto spool webapp right?
There was a problem hiding this comment.
Otherwise remember nensi also added some more extended browser detection one time.
There was a problem hiding this comment.
Right. Makes sense, we can actually use that to simplify.
| <FirefoxPermissionItem | ||
| className="mt-2" | ||
| title="Ads pixel tracking" | ||
| description="Enable pixel tracking for fewer, non-intrusive ads, helping us in keeping the experience free and relevant." |
There was a problem hiding this comment.
Maybe raise with product if they don't want to add people can also buy plus and no ads are shown here?
There was a problem hiding this comment.
Good point. Let me check.
packages/shared/src/lib/func.ts
Outdated
| @@ -95,10 +95,11 @@ export const sortAlphabeticallyByProperty = | |||
| export enum UserAgent { | |||
| Chrome = 'Chrome', | |||
| Edge = 'Edg', // intended to be Edg, not Edge | |||
| Firefox = 'Firefox', // intended to be Edg, not Edge | |||
There was a problem hiding this comment.
Comment shouldn't be here.
| /> | ||
| <div className="flex w-full max-w-[35rem] flex-col items-center gap-4"> | ||
| <span className="h-36 max-w-[16.25rem]"> | ||
| <img src={cloudinaryGenericErrorDark} alt="You declined (FML)" /> |
| > | ||
| You can{' '} | ||
| <button type="button" className="text-text-link" onClick={onGoBack}> | ||
| go back |
There was a problem hiding this comment.
Since the user has already declined, maybe we should also show a link to the webapp as an alternative?
There was a problem hiding this comment.
Good point. I can also raise it to the product team 👀
| if (isFirefox && permission !== FirefoxPermissionType.Accepted) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@idoshamun we are now stopping the send of analytics until it is accepted, only of course for Firefox.
There was a problem hiding this comment.
we just need to make sure this doesn't apply to the firefox browser. just the extension
Changes
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
MI-613
Preview domain
https://mi-631.preview.app.daily.dev