-
-
Notifications
You must be signed in to change notification settings - Fork 386
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
Show Chromium users how to pin extension in toolbar #2983
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! First impressions:
- The nudge needs to support dark mode. The text is invisible at the moment.
- The nudge would ideally account for scrollbars. My scrollbar doesn't autohide so the tail/pointer is off by the scrollbar's amount (I think). Not a blocking issue, but would be nice if we could handle this properly. Dunno yet if there is a good way for us to do so.
- We should focus on teaching the user how to disable Privacy Badger. Something like, 'Pin
this extensionPrivacy Badger to your toolbar for easy access. The Privacy Badger icon will show you how many trackers it blocks on each page. Click on the icon to manage your privacy settings.to the "Disable for this site" button.' But a bit more than this -- let's think about how to combine the "If you think Privacy Badger is breaking a page" text at the bottom with this nudge. The big idea is that we used to tell users about disabling Privacy Badger way down on the welcome page and now we want to do it at the top. Let's also review our previous work in Update first download page #2812 in light of all of this. - We should detect if the user goes through with pinning and auto-close the nudge then.
- We should let the user close the nudge themselves with an "X" maybe.
src/skin/js/firstRun.js
Outdated
@@ -15,4 +15,18 @@ $(function () { | |||
}); | |||
} | |||
}); | |||
|
|||
// Nudge Chrome users to pin the extension to their toolbar | |||
if (navigator && navigator.userAgent && navigator.userAgent.match(/chrome/i)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our goal here is to not show the nudge in browsers where we can auto-pin using the default_area
property, right? That's only Firefox. All Chromium-based browsers (Edge, Opera, Brave, ...) should also get this prompt.
We should avoid checking the UA whenever possible because users change their UA or blank it out among other reasons.
browser.browserAction.getUserSettings()
exists in Firefox and will async return an object with isOnToolbar
set to true
.
chrome.action.getUserSettings()
exists in MV3 Chrome and will async return an object with isOnToolbar
set to false
.
We should probably use this to decide whether to show the nudge or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this change was only for Chrome users but this is a good idea for how to target all browsers except for Firefox! Thank you!
One complication with including Edge, Opera, and Brave is that the icons that users need to click are a little different in each browser. For example, Opera has a box instead of a puzzle piece as the extension icon in the toolbar. To pin on Edge, users select an eye icon instead of a pin icon. So we might need to detect which browser the page is on and load a different image for each browser. That's what Daly did in this PR.
Let's discuss what approach to take for different browsers when we talk about a new design proposal?
I don't love the popup message but I'm still thinking about alternatives. I'm worried the "Disable for this site" reference needs more context or is too specific, since it's not all the popup has. Here's one alternative that's a bit longer:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Left some feedback inline, plus:
- Is the puzzle piece icon also unused?
- The overlay should appear on the left w/ RTL locales. Let's flip the image in the overlay too.
- Does running any new images through https://imageoptim.com/mac help save space?
- We could also use a placeholder for "Disable for this site" in '... access to the "Disable for this site" button'. Then, we won't have to worry about having the translation here and in the popup being in sync.
- I wonder if we should gray out the rest of the page to draw attention to, and to force interaction with the overlay. Kind of like what happens in the popup when
badger.criticalError
is set in the background process. I am not sure if we should, just concerned whether the overlay is too easy to ignore. The orange hero background, plus the "Take the tour" CTA button, plus the social sharing buttons vs. the gray overlay, you know? - Maybe the X should be smaller?
Thanks for the feedback! I removed the unused puzzle piece icon and ran the new images through imageoptim. I like the gray background while the nudge is showing! |
src/skin/js/firstRun.js
Outdated
// Don't show the pin nudge in Firefox because extensions are pinned automatically | ||
// chrome.action is only available in MV3 and chrome.browserAction is only available in <= MV2 | ||
// chrome.browserAction.getUserSettings doesn't exist in MV2 but does in Firefox | ||
let isChromiumMv3 = chrome.action; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!!chrome.action
to make a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Booleans should get snake case names, same as integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more nits and a question.
src/skin/js/firstRun.js
Outdated
// Don't show the pin nudge in Firefox because extensions are pinned automatically | ||
// chrome.action is only available in MV3 and chrome.browserAction is only available in <= MV2 | ||
// chrome.browserAction.getUserSettings doesn't exist in MV2 but does in Firefox | ||
let isChromiumMv3 = chrome.action; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Booleans should get snake case names, same as integers.
src/_locales/en_US/messages.json
Outdated
@@ -359,6 +359,22 @@ | |||
"message": "Privacy Policy", | |||
"description": "Shown at the bottom of the intro page." | |||
}, | |||
"intro_pin_nudge": { | |||
"message": "$START_SPAN_TAG$First, pin Privacy Badger to your toolbar$END_SPAN_TAG$ for easy access to the \"$DISABLE_FOR_SITE$\" button.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "First," be bolded? Not sure, just wondering which one looks/reads better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same ...
02e971d
to
89e214d
Compare
89e214d
to
dd8ca9e
Compare
dd8ca9e
to
0822745
Compare
Encourage Chrome users to pin Privacy Badger to their toolbar by:
Test by running branch in Chrome to see changes
(make runch)
and another browser to confirm nothing changed(make runff)
Seeking feedback on the design and language, especially for the arrow.
Also, following up on #2959 and d1a748c, I fixed a UI bug I noticed with the Share icons on narrow screens:
Part of #2781