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.

Sign up for GitHub

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

Jump to bottom

Adds a version number as per #1088 #1356

Merged
merged 7 commits into from
May 2, 2017
Merged

Adds a version number as per #1088 #1356

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1567efe
adding a version number to the popup
ynasser Apr 29, 2017
09c4343
lint
ynasser Apr 29, 2017
e492e50
addressing feedback
ynasser Apr 30, 2017
e4f27e6
lint
ynasser Apr 30, 2017
e5dcd7a
addressing some PR feedback
ynasser May 1, 2017
664bcf9
actually moving version text to the right spot
ynasser May 2, 2017
33a881b
moving title + subtitle 5px to the right
ynasser May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/_locales/en_US/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@
},
"donate_to_eff": {
"message": "Donate to EFF"
},
"version": {
"message": "version"
}

}
3 changes: 3 additions & 0 deletions src/js/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ function init() {
$("#deactivate_site_btn").hide();
}
});

var version = i18n.getMessage("version") + " " + chrome.runtime.getManifest().version;
$("#version").text(version);
}
$(init);

Expand Down
2 changes: 1 addition & 1 deletion src/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,5 @@
"web_accessible_resources": [
"skin/*",
"icons/*"
]
],
Copy link
Member

@ghostwords ghostwords May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
4 changes: 4 additions & 0 deletions src/skin/popup.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,3 +378,7 @@ font-size: 16px;
#siteControls button {
font-size: 12px;
}

#version{
color: #707070;
}
3 changes: 2 additions & 1 deletion src/skin/popup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ <h1 id="report_title" class="i18n_report_title"></h1>
<div id='privacyBadgerHeader'>
<a id="options" title="options" href='/skin/options.html' class="control-button grey-button" style="display:block" target="_blank"><img src='/icons/options.svg'></a>
<a id="help" title="help" href='/skin/firstRun.html' class="control-button grey-button" style="display:block" target="_blank"><img src='/icons/help.svg'></a>
<div id='title'><img src='/icons/badger-48.png'> <h2><span class="i18n_privacy_badger"></span></h2></div>
<div id='title'><img src='/icons/badger-48.png'> <h2><span class="i18n_privacy_badger"></span></h2></div>
<div class='clear'></div>
</div>
<div id="blockedResourcesContainer">
Expand All @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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:

screenshot from 2017-05-01 11 39 49

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

Copy link
Member

@ghostwords ghostwords May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

pb_css

Copy link
Member

@ghostwords ghostwords May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 ├────┤
└─── ┴────┘

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

screenshot from 2017-05-01 21-31-54

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this look:
pb_moved_to_right

There were moved 5px to the right.

</body>
</html>