Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(358)

Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by martin
Modified:
3 hours, 19 minutes ago
CC:
saroyanm
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 5943 - Implement Updates Page for Adblock Plus extension

Patch Set 1 #

Total comments: 26

Patch Set 2 : Addressed initial round of feedback #

Total comments: 65

Patch Set 3 : Addressed second round of feedback #

Patch Set 4 : Addressed second round of feedback #

Total comments: 32

Patch Set 5 : Addressed third round of feedback #

Total comments: 14

Patch Set 6 : Addressed fourth round of feedback #

Total comments: 14

Patch Set 7 : Addressed fifth round of feedback #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+465 lines, -0 lines) Patch
A locale/en_US/updates.json View 1 2 3 4 5 6 1 chunk +26 lines, -0 lines 2 comments Download
A skin/updates.css View 1 2 3 4 5 6 1 chunk +249 lines, -0 lines 0 comments Download
A skin/updates/appstore-bg.svg View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A skin/updates/base-graphic.svg View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A skin/updates/googleplay-bg.svg View 1 2 3 4 5 1 chunk +41 lines, -0 lines 0 comments Download
A skin/updates/icon-mobile.svg View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A skin/updates/icon-rocket.svg View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A skin/updates/icon-thumbs-up.svg View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A updates.html View 1 2 3 4 5 6 1 chunk +97 lines, -0 lines 3 comments Download
A updates.js View 1 2 3 4 5 6 1 chunk +47 lines, -0 lines 4 comments Download

Messages

Total messages: 18
martin
1 month, 2 weeks ago (2017-10-30 10:37:35 UTC) #1
martin
Hello there, Submitted an initial implementation of the updates page. Will try and address any ...
1 month, 2 weeks ago (2017-10-30 10:39:09 UTC) #2
juliandoucette
Is this supposed to work on mobile? https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29592570/skin/updates.css#newcode11 skin/updates.css:11: @font-face NIT: ...
1 month, 2 weeks ago (2017-10-30 15:30:11 UTC) #3
juliandoucette
(Note: Above are my first impressions. I stopped reviewing after updates.html and the first part ...
1 month, 2 weeks ago (2017-10-30 15:33:46 UTC) #4
martin
Hey all. I (hopefully) addressed the first feedback round. @juliandoucette: from what I know from ...
1 month, 1 week ago (2017-11-03 10:02:15 UTC) #5
juliandoucette
Quick reply. Thank Martin! https://codereview.adblockplus.org/29592569/diff/29592570/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29592570/updates.html#newcode3 updates.html:3: - This file is part ...
1 month, 1 week ago (2017-11-03 11:06:06 UTC) #6
Thomas Greiner
The main points I mentioned are around translations, links and the app store badges. Apart ...
1 month, 1 week ago (2017-11-03 17:40:03 UTC) #7
martin
Hey all, I submitted a second review, addressing Thomas's feedback. Let me know if I ...
1 month, 1 week ago (2017-11-06 16:05:50 UTC) #8
Thomas Greiner
Make sure you're always on the latest revision and avoid reverting changes made by others ...
1 month ago (2017-11-09 13:57:31 UTC) #9
Thomas Greiner
Mostly details to address but we should get the links and store buttons resolved. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/abp-logo.svg ...
1 month ago (2017-11-14 12:25:45 UTC) #10
martin
Pushed a new review, addressing (almost all) comments from the previous one. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates-page/abp-logo.svg File skin/updates-page/abp-logo.svg ...
3 weeks, 3 days ago (2017-11-20 11:47:36 UTC) #11
Thomas Greiner
There are mostly JavaScript related issues left. https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29596582/skin/updates.css#newcode131 skin/updates.css:131: left: 100px; ...
3 weeks, 2 days ago (2017-11-20 17:06:08 UTC) #12
martin
Sending a new patch your way. Let me know if I missed something. https://codereview.adblockplus.org/29592569/diff/29605738/skin/updates.css File ...
2 weeks, 3 days ago (2017-11-26 17:08:41 UTC) #13
Thomas Greiner
https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> On 2017/11/26 17:08:39, martin wrote: ...
2 weeks, 3 days ago (2017-11-27 16:15:52 UTC) #14
martin
Submitted a new review. https://codereview.adblockplus.org/29592569/diff/29612630/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29612630/updates.html#newcode75 updates.html:75: <a href="https://eyeo.to/adblockbrowser/ios/update-page" class="store-button applestore-button"> On ...
1 week, 2 days ago (2017-12-05 14:37:33 UTC) #15
Thomas Greiner
https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css File skin/updates.css (right): https://codereview.adblockplus.org/29592569/diff/29619567/skin/updates.css#newcode78 skin/updates.css:78: cursor: pointer; On 2017/12/05 14:37:31, martin wrote: > On ...
3 days, 4 hours ago (2017-12-11 12:11:12 UTC) #16
martin
https://codereview.adblockplus.org/29592569/diff/29630703/updates.html File updates.html (right): https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77 updates.html:77: <a id="adblock_browser_ios_store" class="store-button applestore-button" target="_blank"> On 2017/12/11 12:11:10, Thomas ...
5 hours, 30 minutes ago (2017-12-14 11:25:17 UTC) #17
Thomas Greiner
3 hours, 19 minutes ago (2017-12-14 13:36:28 UTC) #18
https://codereview.adblockplus.org/29592569/diff/29630703/updates.html
File updates.html (right):

https://codereview.adblockplus.org/29592569/diff/29630703/updates.html#newcode77
updates.html:77: <a id="adblock_browser_ios_store" class="store-button
applestore-button" target="_blank">
On 2017/12/14 11:25:16, martin wrote:
> On 2017/12/11 12:11:10, Thomas Greiner wrote:
> > Coding style: "Separate words in ID and class names by a hyphen."
> > 
> > See
> >
>
https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delim...
> 
> Can I change the `adblock_browser_ios_store` ID? I think this specific ID is
> what makes the doc-link work. 

In the JavaScript file you're retrieving the URL for a doclink ID and assign it
to an HTML element for its ID. So yes, those IDs are independent from each other
and can therefore be different.

https://codereview.adblockplus.org/29592569/diff/29630703/updates.js
File updates.js (right):

https://codereview.adblockplus.org/29592569/diff/29630703/updates.js#newcode44
updates.js:44: setLinks("adblock-browser-text", "https://adblockbrowser.org");
On 2017/12/14 11:25:16, martin wrote:
> On 2017/12/11 12:11:11, Thomas Greiner wrote:
> > Again, let's not hardcode any links in the extension and instead use
> > documentation links.
> 
> Couldn't find a doc-link to "https://adblockbrowser.org"... Is there a way to
> generate one or am I not looking in the right place?

The usual process is to update the spec and then inform Ops about implementing
the redirect on the server-side.

There may already be a process already in place to communicate this spec change
to Ops so Winsley may be able to help you with that.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5