Skip to content
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

Make it more clear PB is an EFF project #2463

Merged
merged 13 commits into from
Oct 23, 2019
Merged

Conversation

ablanathtanalba
Copy link
Contributor

Although there isn't an issue already filed on GitHub for this, per internal discussions we've decided that the extension popup ought to be more clear that Privacy Badger is a project of the EFF. This accomplishes that, as well as some minor design changes that we agreed upon in discussion. Still a WIP as we narrow in on what's within the scope of this PR, as well as possible seeking outside perspective on design change.

@ghostwords ghostwords changed the title Polish Popup UI Make it more clear PB is an EFF project Sep 18, 2019
Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is WIP, but here is some quick feedback.

src/skin/popup.html Outdated Show resolved Hide resolved
src/skin/popup.html Outdated Show resolved Hide resolved
@ghostwords

This comment has been minimized.

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe after fixing the Badger logo, could you add a representative screenshot of the new popup to the description?

@ablanathtanalba

This comment has been minimized.

@ablanathtanalba

This comment has been minimized.

@ablanathtanalba ablanathtanalba marked this pull request as ready for review September 30, 2019 18:02
Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see a vertical scrollbar in Firefox on sites that fill up the tracker list, and the Privacy Badger name is still an image (should be a font).

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Got some feedback.

src/icons/PrivacyBadger-noborder.svg Outdated Show resolved Hide resolved
src/skin/popup.html Outdated Show resolved Hide resolved
src/skin/popup.html Outdated Show resolved Hide resolved
src/skin/popup.html Outdated Show resolved Hide resolved
src/skin/popup.html Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ablanathtanalba ablanathtanalba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious -- I'm actually not able to reproduce the scrollbar error in my environment. I initially had the problem after making the image stack, but was able to fix it by tweaking some padding and margin spacing. I'll keep trying to reproduce it on my machine.

@ghostwords
Copy link
Member

You probably have trouble reproducing because of how macOS handles scrollbars by default.

@ghostwords
Copy link
Member

we should see what fixes we need to make to RTL language hacks

Should also check what impact if any this will have on #1077 and #1113.

@ghostwords
Copy link
Member

Here is what the popup looks like for me after the most recent commit in Firefox on nytimes.com:

Screenshot from 2019-10-02 17:49:07

Note the scrollbar and the extra space in the header.

@ablanathtanalba
Copy link
Contributor Author

ablanathtanalba commented Oct 2, 2019

Thank you for the screenshot! I'm switching to a linux environment for this last task so that I can accurately recreate this scrollbar problem -- even when setting my mac environment to show all scrollbars (not the default), I can't recreate this problem

@ablanathtanalba
Copy link
Contributor Author

I'm still unable to recreate the scrollbar error, in both a linux or a macos environment (and each tested with different resolution settings just in case). For reference, here's a screenshot of visiting nytimes in my ubuntu VM, running a fresh badger instance off this most recent commit:

Screen Shot 2019-10-15 at 5 38 51 PM

I suspect that with the merge of #2479 into master, this will no longer be an issue. I'll circle back to confirm after a rebase and some manual testing

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before/after comparison in Chrome (after merging in master):

popup_comparison

  • The "Share" button grew a bit relative to the other two buttons. Should go back to original size.
  • We lost some margin/padding between the header and the "Privacy Badger detected" text. Should restore.
  • Seems like we should shift "Privacy Badger" down a few pixels so that it's less crowded with "EFF".
  • Should restore the margin/padding above the header. The Help/Share/Options buttons still have the original amount of white space at the top, but the Badger logo and the EFF/Privacy Badger text got pushed up making them feel more cramped.
  • Does the "Privacy Badger" font look heavier on Linux? Can we fix this?

src/skin/images/PrivacyBadger-ChunkFive-Black.svg Outdated Show resolved Hide resolved
Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some RTL locale issues:

Screenshot from 2019-10-16 11:31:53

  • "EFF"/header misalignment
  • version string should be right-aligned

The slider legend icons being misaligned (thanks to the scrollbar) is an existing issue. We don't have to fix it for this PR. I mean, all sliders area elements should be flipped horizontally, but same deal, existing issue, not new to this PR.

@ablanathtanalba
Copy link
Contributor Author

ablanathtanalba commented Oct 22, 2019

As the request for comment goes through the design team's laundry cycles, here are recent updates:

workaround on RTL locales now looks like this. Just a proposed change, still waiting for formal review from design:
RTL_on_macos

Here is english LTR on the same MacOS environment:
LTR_on_macos

And here is the same locale on linux environment:
LTR_on_linux

I cut the margin space again out of the blockedResourcesContainer div in the popup so as to prevent the scrollbar from appearing in linux environments. It seems to me that the tradeoff is either we reduce that space, or the space between the now stacked elements in the header bar (which were pretty cramped before). Open to either, but we now only have 3px of vertical giving room in this current iteration before the scrollbar will reappear in linux environments.

Also, I worked around the discrepancy between the svg icon sizes by for now scaling down the slightly larger one by 2px. Now the share icon is 23px wide, and the other two are at the regular 25. The difference is less jarring from a width perspective, I guess.

Copy link
Member

@ghostwords ghostwords left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

ghostwords added a commit that referenced this pull request Oct 23, 2019
Update the header in the popup with a new look.
@ghostwords ghostwords merged commit 3beb389 into master Oct 23, 2019
@ghostwords ghostwords deleted the new-popup-ui-option-a branch October 23, 2019 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants