Adds a version number as per #1088 #1356

void-elf · 2017-04-29T05:34:01Z

I haven't really written any frontend code before, so I hope this isn't too bad.

ghostwords

Cool! I'd like to request some changes:

Could we use our existing "version" attribute from manifest.json? I'd like to avoid managing two sets of versions if possible.
You should be able to get the version number from the manifest by calling chrome.runtime.getManifest.
Could we restyle the version number to be less conspicuous? Consider all the elements in the popup and their relative importance. I think the version number is useful, but less useful than most of the rest of the popup. Maybe smaller text, some shade of gray, moved to a corner?
Given the availability of Badger in several languages, I think we should make the "v" prefix localizable. My suggestion is to spell it out for clarity, like "version 2017.4.19.1". Should fit if moved to own line in a corner.

void-elf · 2017-04-30T23:17:24Z

Thanks for the feedback. I've addressed some of the changes you requested, but couldn't figure out how to get localization to work. I updated _locales/en_US/messages.json to include version and tried using i18n.getMessage("version") in popup.js to no avail. Is there something more required to get localization to work?

ghostwords

Please see my inline comments.

ghostwords · 2017-05-01T15:38:18Z

src/manifest.json

@@ -93,5 +93,5 @@
  "web_accessible_resources": [
    "skin/*",
    "icons/*"
-  ]
+  ],


The trailing comma makes it invalid JSON (JSON is more constrained than JavaScript). Chrome doesn't let you load the extension; all the tests fail because of this.

ghostwords · 2017-05-01T15:51:10Z

src/skin/popup.html

@@ -80,5 +80,6 @@ <h1 id="report_title" class="i18n_report_title"></h1>
  <center><button id="donate" class="pbButton" style="display:block" target="blank"><span class="i18n_donate_to_eff"></span></button></center>
 </div>
 <div class='clear'></div>
+<div class='footer'><span id="version" class="i18n_version"></span></div>


I like this more, but I have another suggestion: Could we try positioning the version string directly beneath the words Privacy Badger? Mockup attached below:

Here are the suggested changes:

Reduce size of "Privacy Badger"

Move "Privacy Badger" up a little bit to make room for the version string below

Align "Privacy Badger" and version string along left edge

Create white space below the title to give some breathing room to the title and the summary text below the title

I think the localization is working (it says "hallo" instead of "version" because I changed the string in my en_US locale and reloaded the extension to show the new localization).

We should update the locale string however to support languages with different word order. This means using placeholders (#1329), which I'm not sure our current localization setup supports. This isn't a blocker though.

Upon restarting my browser, the localization string "version" started being displayed. I guess it was working all along, but wasn't updating locally for me for some reason.

Regarding your suggested changes, I reduced the size of "Privacy Badger" and moved it up a bit, and aligned the version string along the left edge. I've only used CSS and handful of times before and couldn't figure out how to move the version string up. I tried the same technique I used to move "Privacy Badger" up (adjusting the margin), but it seems to be stuck at this height and won't go any higher:

You should be able to move title/subtitle elements with position: relative and (negative?) top values. Though, could you go over this layout with someone who is more familiar with CSS than us? We are currently floating left a bunch of elements and we want to have these two text rows span the logo:
┌─── ┬────┐
│ logo ├────┤
└─── ┴────┘

Hm, I didn't see your comment before I came back to work on this, and it turned out the missing link was including float for version. Does this most recent version look OK:

That's great! Maybe tweak the amounts of white space a little (add some distance between the logo and the text).

How does this look:

There were moved 5px to the right.

ghostwords · 2017-05-01T16:02:22Z

Also, you could set up your editor to do linting locally and live as you develop. To start with, you could just install the linter and run it manually on the codebase. From the Badger git checkout folder, run npm install, then make lint.

ghostwords · 2017-05-02T14:15:40Z

The failing job looks like an occasional bug in our Firefox test setup.

Could you check up on a few things before we ship?

Does this look the same in Firefox?
Does this make either Badger popup UI is cut off on the bottom in some locales #1077 or Popup cut off when page zoom is set to more than 100% #1113 worse?
What happens if the word for "version" in your language is pretty long? How long is too long?
Do we need to add the new localization string to all locales?

void-elf · 2017-05-02T21:26:14Z

Does this look the same in Firefox?

Yep

Does this make either #1077 or #1113 worse?

I couldn't reproduce those, so I can't confirm that it doesn't make them worse, but my intuition would be no, since we moved the "Privacy Badger" text up to adhere to this layout, and so nothing got pushed down.
┌─── ┬────┐
│ logo ├────┤
└─── ┴────┘

What happens if the word for "version" in your language is pretty long? How long is too long?

I made the string very, very long to check, and it wraps nicely:

And long without wrapping:

Do we need to add the new localization string to all locales?

It's not required for the app to run, as we have a default locale specified in manifest.json. From chrome's i18n documentation: "You don't have to define every string for every supported locale. As long as the default locale's messages.json file has a value for every string, your extension or app will run no matter how sparse a translation is."

We should eventually get this translated into other languages, but I have no idea how we handle that now. Should I make an issue for this?

ghostwords · 2017-05-02T21:41:16Z

Looks good, thank you!

Re translations: I don't think so, although I'm not sure myself what the process is (#1280).

ynasser added 2 commits April 28, 2017 22:24

adding a version number to the popup

1567efe

lint

09c4343

ghostwords self-requested a review April 29, 2017 17:41

ghostwords requested changes Apr 29, 2017

View reviewed changes

ynasser added 2 commits April 30, 2017 15:52

addressing feedback

e492e50

lint

e4f27e6

ghostwords self-requested a review May 1, 2017 15:36

ghostwords requested changes May 1, 2017

View reviewed changes

ynasser added 2 commits May 1, 2017 14:03

addressing some PR feedback

e5dcd7a

actually moving version text to the right spot

664bcf9

moving title + subtitle 5px to the right

33a881b

ghostwords approved these changes May 2, 2017

View reviewed changes

ghostwords merged commit b9a1561 into master May 2, 2017

ghostwords deleted the issue-1088 branch May 2, 2017 21:42

ghostwords added translations ui User interface modifications; related to but not the same as the "ux" label labels Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a version number as per #1088 #1356

Adds a version number as per #1088 #1356

void-elf commented Apr 29, 2017 •

edited

Loading

ghostwords left a comment •

edited

Loading

void-elf commented Apr 30, 2017

ghostwords left a comment

ghostwords May 1, 2017 •

edited

Loading

ghostwords May 1, 2017

ghostwords May 1, 2017 •

edited

Loading

void-elf May 1, 2017

ghostwords May 1, 2017 •

edited

Loading

void-elf May 2, 2017

ghostwords May 2, 2017

void-elf May 2, 2017

ghostwords commented May 1, 2017

ghostwords commented May 2, 2017

void-elf commented May 2, 2017

ghostwords commented May 2, 2017

Adds a version number as per #1088 #1356

Adds a version number as per #1088 #1356

Conversation

void-elf commented Apr 29, 2017 • edited Loading

ghostwords left a comment • edited Loading

Choose a reason for hiding this comment

void-elf commented Apr 30, 2017

ghostwords left a comment

Choose a reason for hiding this comment

ghostwords May 1, 2017 • edited Loading

Choose a reason for hiding this comment

ghostwords May 1, 2017

Choose a reason for hiding this comment

ghostwords May 1, 2017 • edited Loading

Choose a reason for hiding this comment

void-elf May 1, 2017

Choose a reason for hiding this comment

ghostwords May 1, 2017 • edited Loading

Choose a reason for hiding this comment

void-elf May 2, 2017

Choose a reason for hiding this comment

ghostwords May 2, 2017

Choose a reason for hiding this comment

void-elf May 2, 2017

Choose a reason for hiding this comment

ghostwords commented May 1, 2017

ghostwords commented May 2, 2017

void-elf commented May 2, 2017

ghostwords commented May 2, 2017

void-elf commented Apr 29, 2017 •

edited

Loading

ghostwords left a comment •

edited

Loading

ghostwords May 1, 2017 •

edited

Loading

ghostwords May 1, 2017 •

edited

Loading

ghostwords May 1, 2017 •

edited

Loading