Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29559620: Issue 5692 - Create Browser Selector with Browser Detection Component for help.eyeo.com (Closed)

Created:
Sept. 29, 2017, 12:02 p.m. by ire
Modified:
Nov. 3, 2017, 8:45 a.m.
Reviewers:
kzar, juliandoucette
Base URL:
https://hg.adblockplus.org/help.eyeo.com
Visibility:
Public.

Description

Issue 5692 - Create Browser Selector with Browser Detection Component for help.eyeo.com

Patch Set 1 #

Total comments: 7

Patch Set 2 : Added missing translations #

Patch Set 3 : Minor cleanup #

Patch Set 4 : Rebased #

Total comments: 61

Patch Set 5 : Translate autodetected string, change only relevant ua-* classes on body #

Patch Set 6 : SVG enhancement for arrow-icon, correct formatting #

Total comments: 3

Patch Set 7 : Remove test class names from body #

Total comments: 41

Patch Set 8 : Extend CustomSelect, address NITs #

Total comments: 6

Patch Set 9 : Use Object.create() for inheritance, Remove console.log #

Patch Set 10 : Indentation #

Total comments: 3

Patch Set 11 : Use bowser browser names #

Total comments: 2

Patch Set 12 : Re-add browser-select include, add scripts block #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+955 lines, -38 lines) Patch
A globals/browsers.py View 1 chunk +54 lines, -0 lines 0 comments Download
M includes/adblockplus/block-all-ads.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -3 lines 0 comments Download
M includes/adblockplus/remove-a-website-from-the-whitelist.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
A includes/browser-select.tmpl View 1 2 3 4 5 6 7 8 1 chunk +22 lines, -0 lines 0 comments Download
M pages/adblockplus/add-a-filter-list.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -3 lines 0 comments Download
M pages/adblockplus/add-a-website-to-the-whitelist.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/block-malware-sites.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/broken-website.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -4 lines 0 comments Download
M pages/adblockplus/configure-automatic-updates.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M pages/adblockplus/disable-social-media-buttons.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M pages/adblockplus/disable-tracking.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/download-and-install-adblock-plus.md View 1 2 3 4 5 6 7 8 9 10 3 chunks +3 lines, -3 lines 0 comments Download
M pages/adblockplus/hide-the-adblock-plus-icon.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M pages/adblockplus/i-still-see-ads.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/problems-installing.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/remove-a-filter-list.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
M pages/adblockplus/unblock-ablocked-item.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M pages/adblockplus/uninstall-adblock-plus.md View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -3 lines 0 comments Download
A static/img/png/arrow-icon-down-secondary.png View 1 2 3 4 Binary file 0 comments Download
A static/img/svg/arrow-icon-down-secondary.svg View 1 2 3 4 5 1 chunk +12 lines, -0 lines 0 comments Download
M static/js/main.js View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +110 lines, -1 line 0 comments Download
A static/js/vendor/bowser.js View 1 chunk +601 lines, -0 lines 0 comments Download
M static/scss/components/_article.scss View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
A static/scss/components/_browser-select.scss View 1 2 3 4 1 chunk +39 lines, -0 lines 0 comments Download
M static/scss/components/_select.scss View 1 2 3 4 5 6 7 8 2 chunks +62 lines, -1 line 5 comments Download
M static/scss/main.scss View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M templates/article.tmpl View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -1 line 0 comments Download
M templates/minimal.tmpl View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 28
ire
Sept. 29, 2017, 12:02 p.m. (2017-09-29 12:02:38 UTC) #1
ire
Ready for review. This is sort of a combination between issues #5692 and #4923 . ...
Sept. 29, 2017, 12:09 p.m. (2017-09-29 12:09:18 UTC) #2
ire
Also, I didn't really handle the no-js state. Since the no-js state is that all ...
Sept. 29, 2017, 12:11 p.m. (2017-09-29 12:11:39 UTC) #3
ire
https://codereview.adblockplus.org/29559620/diff/29559621/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29559621/includes/browser-select.tmpl#newcode12 includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png" srcset="/img/svg/logo-abp.svg 2x"> Also, forgot to mention I ...
Sept. 29, 2017, 12:13 p.m. (2017-09-29 12:13:24 UTC) #4
ire
https://codereview.adblockplus.org/29559620/diff/29559621/pages/index.md File pages/index.md (right): https://codereview.adblockplus.org/29559620/diff/29559621/pages/index.md#newcode9 pages/index.md:9: <? include browser-select ?> On 2017/09/29 12:09:17, ire wrote: ...
Oct. 11, 2017, 5:21 p.m. (2017-10-11 17:21:46 UTC) #5
juliandoucette
This is cool. https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: NIT: Regarding the no-js state of ...
Oct. 13, 2017, 1:39 p.m. (2017-10-13 13:39:57 UTC) #6
juliandoucette
typo *blushes* https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#newcode263 static/js/main.js:263: document.body.className = "ua-"+browser; On 2017/10/13 13:39:55, juliandoucette ...
Oct. 13, 2017, 1:41 p.m. (2017-10-13 13:41:36 UTC) #7
ire
Thanks Julian! Uploaded a new patch :) https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: On 2017/10/13 ...
Oct. 17, 2017, 3:03 p.m. (2017-10-17 15:03:36 UTC) #8
juliandoucette
https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: On 2017/10/17 15:03:33, ire wrote: > On 2017/10/13 13:39:55, ...
Oct. 18, 2017, 3:04 p.m. (2017-10-18 15:04:12 UTC) #9
ire
New patch ready :) https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: On 2017/10/18 15:04:10, juliandoucette wrote: ...
Oct. 20, 2017, 1:40 p.m. (2017-10-20 13:40:49 UTC) #10
juliandoucette
This doesn't seem to be hiding properly atm? Also NIT it errors if an invalid ...
Oct. 23, 2017, 1:40 p.m. (2017-10-23 13:40:07 UTC) #11
juliandoucette
https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl#newcode46 templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome"> I'm guessing that this ...
Oct. 23, 2017, 1:43 p.m. (2017-10-23 13:43:12 UTC) #12
ire
On 2017/10/23 13:40:07, juliandoucette wrote: > This doesn't seem to be hiding properly atm? I ...
Oct. 23, 2017, 4:13 p.m. (2017-10-23 16:13:47 UTC) #13
ire
https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl#newcode46 templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome"> On 2017/10/23 13:43:12, juliandoucette ...
Oct. 23, 2017, 4:13 p.m. (2017-10-23 16:13:54 UTC) #14
ire
https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl File templates/minimal.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl#newcode46 templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome"> On 2017/10/23 16:13:53, ire ...
Oct. 23, 2017, 4:15 p.m. (2017-10-23 16:15:40 UTC) #15
juliandoucette
All minor issues. https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: Suggest: <fieldset>? https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-select.tmpl#newcode12 includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png" ...
Oct. 24, 2017, 12:12 p.m. (2017-10-24 12:12:30 UTC) #16
ire
New patch ready https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-select.tmpl File includes/browser-select.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-select.tmpl#newcode1 includes/browser-select.tmpl:1: On 2017/10/24 12:12:27, juliandoucette wrote: > ...
Oct. 27, 2017, 10:23 a.m. (2017-10-27 10:23:00 UTC) #17
juliandoucette
https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode230 static/js/main.js:230: } On 2017/10/27 10:22:57, ire wrote: > On 2017/10/24 ...
Oct. 31, 2017, 1:20 p.m. (2017-10-31 13:20:00 UTC) #18
juliandoucette
@Dave please find question below. https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode274 static/js/main.js:274: var selectedItem = this.browserSelect ...
Oct. 31, 2017, 1:24 p.m. (2017-10-31 13:24:03 UTC) #19
kzar
https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode274 static/js/main.js:274: var selectedItem = this.browserSelect On 2017/10/31 13:24:02, juliandoucette wrote: ...
Oct. 31, 2017, 2:22 p.m. (2017-10-31 14:22:32 UTC) #20
ire
New patch https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode230 static/js/main.js:230: } On 2017/10/31 13:19:57, juliandoucette wrote: > ...
Nov. 1, 2017, 12:36 p.m. (2017-11-01 12:36:50 UTC) #21
juliandoucette
https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode237 static/js/main.js:237: if (bowser.chrome) browser = "chrome"; On 2017/11/01 12:36:47, ire ...
Nov. 1, 2017, 12:49 p.m. (2017-11-01 12:49:22 UTC) #22
ire
https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#newcode237 static/js/main.js:237: if (bowser.chrome) browser = "chrome"; On 2017/11/01 12:49:21, juliandoucette ...
Nov. 1, 2017, 5:20 p.m. (2017-11-01 17:20:13 UTC) #23
juliandoucette
I'm seeing "[Browser selector]" on article pages. https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.tmpl#newcode56 templates/article.tmpl:56: <script src="/js/vendor/bowser.js"></script> ...
Nov. 2, 2017, 1:10 p.m. (2017-11-02 13:10:57 UTC) #24
ire
On 2017/11/02 13:10:57, juliandoucette wrote: > I'm seeing "[Browser selector]" on article pages. Oops. Fixed.
Nov. 2, 2017, 2:31 p.m. (2017-11-02 14:31:05 UTC) #25
ire
https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.tmpl File templates/article.tmpl (right): https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.tmpl#newcode56 templates/article.tmpl:56: <script src="/js/vendor/bowser.js"></script> On 2017/11/02 13:10:56, juliandoucette wrote: > suggest: ...
Nov. 2, 2017, 2:31 p.m. (2017-11-02 14:31:15 UTC) #26
juliandoucette
LGTM + Suggestions Suggestions may be addressed separately or ignored. https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/components/_select.scss File static/scss/components/_select.scss (right): https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/components/_select.scss#newcode106 ...
Nov. 2, 2017, 3:22 p.m. (2017-11-02 15:22:21 UTC) #27
ire
Nov. 3, 2017, 8:45 a.m. (2017-11-03 08:45:37 UTC) #28
https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/compone...
static/scss/components/_select.scss:106: .custom-select-option
On 2017/11/02 15:22:19, juliandoucette wrote:
> suggest: change background-color on hove/active/focus of select and options

I'll probably handle this in the other issue handling hover/active/focus state
for the accordion

https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/compone...
static/scss/components/_select.scss:111: // Custom Dropdown (extends the custom
select element)
On 2017/11/02 15:22:18, juliandoucette wrote:
> suggest: category comment (underline it)

Will change all comments in a separate issue

Powered by Google App Engine
This is Rietveld