feat: firefox extension permission ui #3864

sshanzel · 2024-11-21T14:03:25Z

Changes

Check whether the extension is Firefox.
- If so, we check cache whether the user has accepted the permission popup.
- If not, show the prompt.

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

vercel · 2024-11-21T14:03:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
daily-webapp	🛑 Canceled (Inspect)		Nov 27, 2024 2:24pm

1 Skipped Deployment

Name	Status	Preview	Updated (UTC)
storybook	⬜️ Ignored (Inspect)		Nov 27, 2024 2:24pm

packages/extension/src/newtab/App.tsx

AmarTrebinjac

Looking good! 👏🏼

Left a few comments for consideration 😊

AmarTrebinjac · 2024-11-21T14:52:30Z

packages/shared/src/lib/func.ts

+export const checkIsFirefox = (): boolean =>
+  typeof globalThis?.navigator !== 'undefined' &&
+  /firefox/i.test(globalThis?.navigator.userAgent);
+


Should we maybe add "firefox" to the UserAgent enum and use checkIsBrowser instead?

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.

AmarTrebinjac · 2024-11-21T14:55:03Z

packages/extension/src/permission/FirefoxPermissionDeclined.tsx

+          You can{' '}
+          <button type="button" className="text-text-link" onClick={onGoBack}>
+            go back
+          </button>{' '}


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 {' '}

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 👀

rebelchris

Don't we want to do all this in extension only maybe?
As they are the only ones complaining, not specific firefox

packages/extension/src/newtab/App.tsx

rebelchris · 2024-11-22T07:38:45Z

packages/extension/src/newtab/App.tsx

@@ -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);


Why don't you use the existing one?
isFirefoxExtension?

I think in this new flow we don't auto spool webapp right?

Otherwise remember nensi also added some more extended browser detection one time.

Right. Makes sense, we can actually use that to simplify.

rebelchris · 2024-11-22T07:39:59Z

packages/extension/src/permission/FirefoxPermission.tsx

+        <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."


Maybe raise with product if they don't want to add people can also buy plus and no ads are shown here?

Good point. Let me check.

rebelchris · 2024-11-22T07:40:47Z

packages/shared/src/lib/func.ts

@@ -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


Comment shouldn't be here.

omBratteng · 2024-11-22T09:21:39Z

packages/extension/src/permission/FirefoxPermissionDeclined.tsx

+      />
+      <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)" />


(FML) 😂

omBratteng · 2024-11-22T09:22:36Z

packages/extension/src/permission/FirefoxPermissionDeclined.tsx

+        >
+          You can{' '}
+          <button type="button" className="text-text-link" onClick={onGoBack}>
+            go back


Since the user has already declined, maybe we should also show a link to the webapp as an alternative?

Good point. I can also raise it to the product team 👀

Added! 🚀

sshanzel · 2024-11-25T13:13:02Z

packages/shared/src/hooks/log/useLogQueue.ts

+        if (isFirefox && permission !== FirefoxPermissionType.Accepted) {
+          return;
+        }


@idoshamun we are now stopping the send of analytics until it is accepted, only of course for Firefox.

we just need to make sure this doesn't apply to the firefox browser. just the extension

Yes. The isFirefox variable checks the process.env.TARGET_BROWSER which is only applied to extension builds.

Let's change the naming in that case, you can already see it's confusing :)

AmarTrebinjac

Looks good to me, just one comment on the copytext, but non-blocking!

🚀

AmarTrebinjac · 2024-11-26T12:11:41Z

packages/extension/src/permission/FirefoxPermission.tsx

+          color={TypographyColor.Tertiary}
+        >
+          Ready to go? 🟢 Give us the green signal by accepting the below
+          permissions. Enhance your daily.dev experience with personalized


"by accepting the permissions below" is more natural sounding.

rebelchris

Just naming improvements, otherwise good so approving already

rebelchris · 2024-11-26T12:26:40Z

packages/extension/src/newtab/App.tsx

@@ -106,6 +89,7 @@ function InternalApp(): ReactElement {
  const isPageReady =
    (growthbook?.ready && router?.isReady && isAuthReady) || isTesting;
  const shouldRedirectOnboarding = !user && isPageReady && !isTesting;
+  const isFirefox = process.env.TARGET_BROWSER === 'firefox';


We should probably globally export isFirefoxExtension and maybe make it more explicit.
We only target firefox extension, reading this might make you think otherwise.

rebelchris · 2024-11-26T12:27:23Z

packages/shared/src/hooks/log/useLogQueue.ts

@@ -31,6 +36,10 @@ export default function useLogQueue({
  queueRef: MutableRefObject<LogEvent[]>;
  sendBeacon: () => void;
 } {
+  const isFirefox = process.env.TARGET_BROWSER === 'firefox';


Same as above, make it isFirefoxExtension and global imported?

rebelchris · 2024-11-26T12:27:43Z

packages/shared/src/hooks/log/useLogQueue.ts

+        if (isFirefox && permission !== FirefoxPermissionType.Accepted) {
+          return;
+        }


Let's change the naming in that case, you can already see it's confusing :)

sshanzel added 2 commits November 21, 2024 21:45

feat: firefox extension permission ui

fc4323f

refactor: return early only if firefox

8aeba5e

sshanzel requested a review from a team as a code owner November 21, 2024 14:03

sshanzel requested review from rebelchris, omBratteng, ilasw, capJavert, nensidosari and AmarTrebinjac November 21, 2024 14:03

refactor: updated component name

7624474

vercel bot deployed to Preview – daily-webapp November 21, 2024 14:04 View deployment

vercel bot deployed to Preview – daily-webapp November 21, 2024 14:06 View deployment

sshanzel commented Nov 21, 2024

View reviewed changes

packages/extension/src/newtab/App.tsx Show resolved Hide resolved

Merge branch 'main' into MI-631

078de4b

vercel bot deployed to Preview – daily-webapp November 21, 2024 14:19 View deployment

AmarTrebinjac requested changes Nov 21, 2024

View reviewed changes

refactor: usage of existing function

c4466e4

sshanzel requested a review from AmarTrebinjac November 22, 2024 07:16

vercel bot deployed to Preview – daily-webapp November 22, 2024 07:18 View deployment

Merge branch 'main' into MI-631

b1cc52d

vercel bot deployed to Preview – daily-webapp November 22, 2024 07:27 View deployment

rebelchris requested changes Nov 22, 2024

View reviewed changes

refactor: usage of env variable

bd8e02b

vercel bot deployed to Preview – daily-webapp November 22, 2024 07:47 View deployment

omBratteng reviewed Nov 22, 2024

View reviewed changes

Merge branch 'main' into MI-631

adcaba3

vercel bot deployed to Preview – daily-webapp November 22, 2024 12:51 View deployment

feat: introduce a link to web app

050a83a

sshanzel added 4 commits November 25, 2024 20:26

refactor: unnecessary modal

eff0678

fix: installation from extension

872fc1b

fix: redirect to onboarding on accept

f33edee

fix: do not track analytics until permitted

f30809d

vercel bot deployed to Preview – daily-webapp November 25, 2024 12:56 View deployment

sshanzel commented Nov 25, 2024

View reviewed changes

fix: target blank

a729b92

vercel bot deployed to Preview – daily-webapp November 25, 2024 13:37 View deployment

Merge branch 'main' into MI-631

7ce5ea6

vercel bot had a problem deploying to Preview – storybook November 25, 2024 14:29 Failure

vercel bot deployed to Preview – daily-webapp November 25, 2024 14:30 View deployment

fix: centered content

81bc0f1

vercel bot deployed to Preview – daily-webapp November 25, 2024 14:39 View deployment

AmarTrebinjac approved these changes Nov 26, 2024

View reviewed changes

rebelchris approved these changes Nov 26, 2024

View reviewed changes

fix: pr comments

1e248de

vercel bot deployed to Preview – daily-webapp November 27, 2024 13:15 View deployment

Merge branch 'main' into MI-631

1800963

vercel bot deployed to Preview – storybook November 27, 2024 13:23 View deployment

vercel bot deployed to Preview – daily-webapp November 27, 2024 13:23 View deployment

fix: analytics

5b159e7

vercel bot deployed to Preview – daily-webapp November 27, 2024 13:29 View deployment

fix: logo position

5092435

vercel bot deployed to Preview – daily-webapp November 27, 2024 14:12 View deployment

Merge branch 'main' into MI-631

b51ee31

vercel bot deployed to Preview – daily-webapp November 27, 2024 14:16 View deployment

Merge branch 'main' into MI-631

6a558da

vercel bot deployed to Preview – daily-webapp November 27, 2024 14:24 View deployment

sshanzel merged commit 1693d38 into main Nov 27, 2024
10 checks passed

sshanzel deleted the MI-631 branch November 27, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: firefox extension permission ui #3864

feat: firefox extension permission ui #3864

sshanzel commented Nov 21, 2024 •

edited by github-actions bot

Loading

vercel bot commented Nov 21, 2024 •

edited

Loading

AmarTrebinjac left a comment

AmarTrebinjac Nov 21, 2024

sshanzel Nov 22, 2024 •

edited

Loading

AmarTrebinjac Nov 21, 2024 •

edited

Loading

sshanzel Nov 22, 2024

rebelchris left a comment

rebelchris Nov 22, 2024

rebelchris Nov 22, 2024

sshanzel Nov 22, 2024

rebelchris Nov 22, 2024

sshanzel Nov 22, 2024

rebelchris Nov 22, 2024

omBratteng Nov 22, 2024

omBratteng Nov 22, 2024

sshanzel Nov 22, 2024

sshanzel Nov 22, 2024

sshanzel Nov 25, 2024

idoshamun Nov 25, 2024

sshanzel Nov 26, 2024

idoshamun Nov 26, 2024

rebelchris Nov 26, 2024

AmarTrebinjac left a comment

AmarTrebinjac Nov 26, 2024

rebelchris left a comment

rebelchris Nov 26, 2024

rebelchris Nov 26, 2024

rebelchris Nov 26, 2024

feat: firefox extension permission ui #3864

feat: firefox extension permission ui #3864

Conversation

sshanzel commented Nov 21, 2024 • edited by github-actions bot Loading

Changes

Events

Experiment

Manual Testing

Preview domain

vercel bot commented Nov 21, 2024 • edited Loading

AmarTrebinjac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshanzel Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

AmarTrebinjac Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebelchris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmarTrebinjac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebelchris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshanzel commented Nov 21, 2024 •

edited by github-actions bot

Loading

vercel bot commented Nov 21, 2024 •

edited

Loading

sshanzel Nov 22, 2024 •

edited

Loading

AmarTrebinjac Nov 21, 2024 •

edited

Loading