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

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

[ire, 2017-09-29 12:09:18]

Ready for review.

This is sort of a combination between issues #5692 and #4923 . I realised that
for the latter, I didn't need to start from scratch with creating the component
since we already had the select. Let me know if you would prefer I separate them
into separate reviews.

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#newc...
pages/index.md:9: <? include browser-select ?>
Will move this to the article template when that review is done or rebase on top
of that review at a later stage

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js#n...
static/js/main.js:57: this.DEFAULT_BROWSER = "chrome";
This is an arbitrary decision. We can change this based on browser usage
statistics and make this whatever is our most commonly used browser.

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js#n...
static/js/main.js:83: if (bowser.chrome) browser = "chrome";
I decided to go with bowser unless we have some other preferred way of detecting
browser

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js#n...
static/js/main.js:139: // Close Select
I think I should refactor the way the custom select was created so I can call a
"close" function instead of having to do it manually here.

I will create a separate issue for that then refactor this when that is done.

[ire, 2017-09-29 12:11:39]

Also, I didn't really handle the no-js state. Since the no-js state is that all
the articles will be shown (for all browsers), should we even show the browser
selector at all?

[ire, 2017-09-29 12:13:24]

https://codereview.adblockplus.org/29559620/diff/29559621/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29559621/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
Also, forgot to mention I have been using the ABP logo because Jeen hasn't yet
given me browser logos. Will update when she does.

[ire, 2017-10-11 17:21:46]

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#newc...
pages/index.md:9: <? include browser-select ?>
On 2017/09/29 12:09:17, ire wrote:
> Will move this to the article template when that review is done or rebase on
top
> of that review at a later stage

Done.

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29559621/static/js/main.js#n...
static/js/main.js:139: // Close Select
On 2017/09/29 12:09:17, ire wrote:
> I think I should refactor the way the custom select was created so I can call
a
> "close" function instead of having to do it manually here.
> 
> I will create a separate issue for that then refactor this when that is done.

Done.

[juliandoucette, 2017-10-13 13:39:57]

This is cool.

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:1: 
NIT: Regarding the no-js state of this component:

1. You could hide it entirely
2. You could add <a href="#platform-${browser}"></a> around each option
    a. And align the label or the dropdown top
    b. And show one option per line; preferably

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:11: <li class="custom-select-option" role="button"
tabindex="0" data-value="{{ browser.id }}">
NIT/Suggest: role="menuitemradio" aria-checked="false"

Source:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-2/menub...

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
Note: Assuming you will replace these icons

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:13: {{ browser.name | translate(browser.id +
"-name") }}
Missing (autodetected) label?

(See comment js file)

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#n...
static/js/main.js:206: function BrowserSelect(select)
Note: You could extend CustomSelect's prototype (e.g. BrowserSelect.prototype =
extend({}, CustomSelect.prototype)) and apply CustomSelect (e.g.
CustomSelect.apply(this, [select]) and use this.select instead of
this.browserSelect.

What you did here is fine too.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:217: 
NIT: You should be able to click-away too (click elsewhere to close the
dropdown)?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
Wait, what? How can browser have properties if it was just defined as undefined?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:263: document.body.className = "ua-"+browser;
What if there are not related classes attached to the body later?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:268: .browserSelect
NIT: spaces around +?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:274: selectedOption.innerHTML += "<span
class='muted'>(autodetected)</span>";
This isn't translated. I suggest translating it in the HTML and show/hiding or
add/removing it.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:287: if (event.keyCode && !IS_ENTER_KEY) return;
Why check event.keyCode here?

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_browser-select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:20: margin-right: $md;
NIT/Suggest: 1em?

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:25: margin-right: auto;
SuperNIT: Shorthand margin would be shorter.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:29: #browser-select
NIT: If you use the floating box model then you won't have to deal with the
extra whitespace. Especially on small screens (if this dropdown breaks to the
next line).

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:109: .custom-select-dropdown img
NIT: I think this image is supposed to be larger?

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:123: height: 40px;
NIT: It's inconsistent to mix px and em here

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:141: width: 2em;
NIT: This looks wider in the mockups? 3em? (one width, one padding on each side)

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:143: border-left: 2px solid $gray-medium;
NIT: This looks like 1px in the mockups? (The same applies elsewhere)

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:144: background-image:
url(/img/png/arrow-down-icon-secondary.png);
NIT: Can we use SVG or 2x PNG and fallback to 1x png somehow?

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:167: line-height: 40px;
NIT: It's inconsistent to mix px and em here

[juliandoucette, 2017-10-13 13:41:36]

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#n...
static/js/main.js:263: document.body.className = "ua-"+browser;
On 2017/10/13 13:39:55, juliandoucette wrote:
> What if there are not related classes attached to the body later?

unrelated*

[ire, 2017-10-17 15:03:36]

Thanks Julian! Uploaded a new patch :)

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:1: 
On 2017/10/13 13:39:55, juliandoucette wrote:
> NIT: Regarding the no-js state of this component:
> 
> 1. You could hide it entirely
> 2. You could add <a href="#platform-${browser}"></a> around each option
>     a. And align the label or the dropdown top
>     b. And show one option per line; preferably

I think I will hide it entirely. Because each section has the classname
.platform-${browser}, not the unique ID

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:1: 
On 2017/10/13 13:39:55, juliandoucette wrote:
> NIT: Regarding the no-js state of this component:
> 
> 1. You could hide it entirely
> 2. You could add <a href="#platform-${browser}"></a> around each option
>     a. And align the label or the dropdown top
>     b. And show one option per line; preferably

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:11: <li class="custom-select-option" role="button"
tabindex="0" data-value="{{ browser.id }}">
On 2017/10/13 13:39:55, juliandoucette wrote:
> NIT/Suggest: role="menuitemradio" aria-checked="false"
> 
> Source:
>
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-2/menub...

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
On 2017/10/13 13:39:55, juliandoucette wrote:
> Note: Assuming you will replace these icons

Yes I mentioned that I am still waiting on the icons in a previous comment.

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:13: {{ browser.name | translate(browser.id +
"-name") }}
On 2017/10/13 13:39:55, juliandoucette wrote:
> Missing (autodetected) label?
> 
> (See comment js file)

Acknowledged.

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#n...
static/js/main.js:206: function BrowserSelect(select)
On 2017/10/13 13:39:56, juliandoucette wrote:
> Note: You could extend CustomSelect's prototype (e.g. BrowserSelect.prototype
=
> extend({}, CustomSelect.prototype)) and apply CustomSelect (e.g.
> CustomSelect.apply(this, [select]) and use this.select instead of
> this.browserSelect.
> 
> What you did here is fine too.

Cool. I'll keep what i did here then.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:217: 
On 2017/10/13 13:39:56, juliandoucette wrote:
> NIT: You should be able to click-away too (click elsewhere to close the
> dropdown)?

Yes, I will fix this in a separate issue

https://issues.adblockplus.org/ticket/5868#ticket

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/13 13:39:55, juliandoucette wrote:
> Wait, what? How can browser have properties if it was just defined as
undefined?

You can override previously defined (or undefined) properties in this way

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:263: document.body.className = "ua-"+browser;
On 2017/10/13 13:39:55, juliandoucette wrote:
> What if there are not related classes attached to the body later?

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:268: .browserSelect
On 2017/10/13 13:39:55, juliandoucette wrote:
> NIT: spaces around +?

I thought we don't put spaces within the ()?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:274: selectedOption.innerHTML += "<span
class='muted'>(autodetected)</span>";
On 2017/10/13 13:39:55, juliandoucette wrote:
> This isn't translated. I suggest translating it in the HTML and show/hiding or
> add/removing it.

I place it in a template so I wouldn't have to have it in the HTML for all items

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:287: if (event.keyCode && !IS_ENTER_KEY) return;
On 2017/10/13 13:39:56, juliandoucette wrote:
> Why check event.keyCode here?

Because this function is also relevant for clicks. I'm checking here to return
only if it was a keyDown event AND the key wasn't the enter key.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_browser-select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:20: margin-right: $md;
On 2017/10/13 13:39:56, juliandoucette wrote:
> NIT/Suggest: 1em?

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:25: margin-right: auto;
On 2017/10/13 13:39:56, juliandoucette wrote:
> SuperNIT: Shorthand margin would be shorter.

(Same answer I gave in the other review about being an overriding thing)

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:29: #browser-select
On 2017/10/13 13:39:56, juliandoucette wrote:
> NIT: If you use the floating box model then you won't have to deal with the
> extra whitespace. Especially on small screens (if this dropdown breaks to the
> next line).

Sorry, extra space where? When this moves onto its own line it should 100% width

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:109: .custom-select-dropdown img
On 2017/10/13 13:39:57, juliandoucette wrote:
> NIT: I think this image is supposed to be larger?

I think you're right. I would like to change this: 1. When I have the correct
icons and/or 2: During the issue about fixing alignment for the .heading-icon

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:123: height: 40px;
On 2017/10/13 13:39:57, juliandoucette wrote:
> NIT: It's inconsistent to mix px and em here

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:141: width: 2em;
On 2017/10/13 13:39:57, juliandoucette wrote:
> NIT: This looks wider in the mockups? 3em? (one width, one padding on each
side)

Done.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:143: border-left: 2px solid $gray-medium;
On 2017/10/13 13:39:57, juliandoucette wrote:
> NIT: This looks like 1px in the mockups? (The same applies elsewhere)

We changed it to 2px because you said it was too thin when we used it in the
footer. (since this extends the .custom-select

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:144: background-image:
url(/img/png/arrow-down-icon-secondary.png);
On 2017/10/13 13:39:56, juliandoucette wrote:
> NIT: Can we use SVG or 2x PNG and fallback to 1x png somehow?

I know it's possible to set multiple background-image urls (e.g.
`background-image: url(one.png), url(two.png)`), but these will be overlaid on
top of each other. I'm not sure what other methods there are to doing this. Will
look it up

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:167: line-height: 40px;
On 2017/10/13 13:39:56, juliandoucette wrote:
> NIT: It's inconsistent to mix px and em here

Done.

[juliandoucette, 2017-10-18 15:04:12]

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:1: 
On 2017/10/17 15:03:33, ire wrote:
> On 2017/10/13 13:39:55, juliandoucette wrote:
> > NIT: Regarding the no-js state of this component:
> > 
> > 1. You could hide it entirely
> > 2. You could add <a href="#platform-${browser}"></a> around each option
> >     a. And align the label or the dropdown top
> >     b. And show one option per line; preferably
> 
> I think I will hide it entirely. Because each section has the classname
> .platform-${browser}, not the unique ID

NIT: Adding an id (too) would make them deep linkable without JS.

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
On 2017/10/17 15:03:33, ire wrote:
> On 2017/10/13 13:39:55, juliandoucette wrote:
> > Note: Assuming you will replace these icons
> 
> Yes I mentioned that I am still waiting on the icons in a previous comment.

Acknowledged.

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#n...
static/js/main.js:217: 
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:56, juliandoucette wrote:
> > NIT: You should be able to click-away too (click elsewhere to close the
> > dropdown)?
> 
> Yes, I will fix this in a separate issue
> 
> https://issues.adblockplus.org/ticket/5868#ticket

Acknowledged.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:55, juliandoucette wrote:
> > Wait, what? How can browser have properties if it was just defined as
> undefined?
> 
> You can override previously defined (or undefined) properties in this way

Can you elaborate / explain when/how this variable is not undefined?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:247: else if (bowser.safari) browser = "safari";
Why check each browser property and not *just* browser.name?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:268: .browserSelect
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:55, juliandoucette wrote:
> > NIT: spaces around +?
> 
> I thought we don't put spaces within the ()?

We don't put spaces after the opening brace and before the closing brace AFAIK
(I can't find a reference in our code-style).

(1 + 1) //good
( 1 + 1 ) //bad

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:274: selectedOption.innerHTML += "<span
class='muted'>(autodetected)</span>";
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:55, juliandoucette wrote:
> > This isn't translated. I suggest translating it in the HTML and show/hiding
or
> > add/removing it.
> 
> I place it in a template so I wouldn't have to have it in the HTML for all
items

You mean you will place it in a template? Or something else?

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:287: if (event.keyCode && !IS_ENTER_KEY) return;
On 2017/10/17 15:03:33, ire wrote:
> On 2017/10/13 13:39:56, juliandoucette wrote:
> > Why check event.keyCode here?
> 
> Because this function is also relevant for clicks. I'm checking here to return
> only if it was a keyDown event AND the key wasn't the enter key. 

Acknowledged. I was confused.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_browser-select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:25: margin-right: auto;
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:56, juliandoucette wrote:
> > SuperNIT: Shorthand margin would be shorter.
> 
> (Same answer I gave in the other review about being an overriding thing)

Acknowledged.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:29: #browser-select
On 2017/10/17 15:03:34, ire wrote:
> On 2017/10/13 13:39:56, juliandoucette wrote:
> > NIT: If you use the floating box model then you won't have to deal with the
> > extra whitespace. Especially on small screens (if this dropdown breaks to
the
> > next line).
> 
> Sorry, extra space where? When this moves onto its own line it should 100%
width

No idea :/ . Doesn't seem to be an issue to me. Sorry.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:109: .custom-select-dropdown img
On 2017/10/17 15:03:35, ire wrote:
> On 2017/10/13 13:39:57, juliandoucette wrote:
> > NIT: I think this image is supposed to be larger?
> 
> I think you're right. I would like to change this: 1. When I have the correct
> icons and/or 2: During the issue about fixing alignment for the .heading-icon 

Acknowledged.

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:144: background-image:
url(/img/png/arrow-down-icon-secondary.png);
On 2017/10/17 15:03:35, ire wrote:
> On 2017/10/13 13:39:56, juliandoucette wrote:
> > NIT: Can we use SVG or 2x PNG and fallback to 1x png somehow?
> 
> I know it's possible to set multiple background-image urls (e.g.
> `background-image: url(one.png), url(two.png)`), but these will be overlaid on
> top of each other. I'm not sure what other methods there are to doing this.
Will
> look it up

https://css-tricks.com/svg-fallbacks/#article-header-id-4

[ire, 2017-10-20 13:40:49]

New patch ready :)

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29573824/includes/browser-se...
includes/browser-select.tmpl:1: 
On 2017/10/18 15:04:10, juliandoucette wrote:
> On 2017/10/17 15:03:33, ire wrote:
> > On 2017/10/13 13:39:55, juliandoucette wrote:
> > > NIT: Regarding the no-js state of this component:
> > > 
> > > 1. You could hide it entirely
> > > 2. You could add <a href="#platform-${browser}"></a> around each option
> > >     a. And align the label or the dropdown top
> > >     b. And show one option per line; preferably
> > 
> > I think I will hide it entirely. Because each section has the classname
> > .platform-${browser}, not the unique ID
> 
> NIT: Adding an id (too) would make them deep linkable without JS.

The issue with adding an id was because of sections that apply to multiple
platforms. And there are sometimes multiple sections for one platform too.

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#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/18 15:04:10, juliandoucette wrote:
> On 2017/10/17 15:03:34, ire wrote:
> > On 2017/10/13 13:39:55, juliandoucette wrote:
> > > Wait, what? How can browser have properties if it was just defined as
> > undefined?
> > 
> > You can override previously defined (or undefined) properties in this way
> 
> Can you elaborate / explain when/how this variable is not undefined?

Sure!

I create a pen to illustrate this -
https://codepen.io/ire/pen/MExoQM?editors=0011

Basically, variables created with var/let can have their values updated in this
way. You can initialise a variable with one value, then update it later on.
Then, depending when the variable reference is used, the value can be different.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:247: else if (bowser.safari) browser = "safari";
On 2017/10/18 15:04:10, juliandoucette wrote:
> Why check each browser property and not *just* browser.name?

I felt it wouldn't be as reliable/steady as this. I couldn't find a set list of
the browser names without having to go into the bowser.js code.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:268: .browserSelect
On 2017/10/18 15:04:11, juliandoucette wrote:
> On 2017/10/17 15:03:34, ire wrote:
> > On 2017/10/13 13:39:55, juliandoucette wrote:
> > > NIT: spaces around +?
> > 
> > I thought we don't put spaces within the ()?
> 
> We don't put spaces after the opening brace and before the closing brace AFAIK
> (I can't find a reference in our code-style).
> 
> (1 + 1) //good
> ( 1 + 1 ) //bad

Alright, makes sense. Done.

https://codereview.adblockplus.org/29559620/diff/29573824/static/js/main.js#n...
static/js/main.js:274: selectedOption.innerHTML += "<span
class='muted'>(autodetected)</span>";
On 2017/10/18 15:04:10, juliandoucette wrote:
> On 2017/10/17 15:03:34, ire wrote:
> > On 2017/10/13 13:39:55, juliandoucette wrote:
> > > This isn't translated. I suggest translating it in the HTML and
show/hiding
> or
> > > add/removing it.
> > 
> > I place it in a template so I wouldn't have to have it in the HTML for all
> items
> 
> You mean you will place it in a template? Or something else?

Sorry I meant to say "I will place it in a template" (as I have done)

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_browser-select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_browser-select.scss:29: #browser-select
On 2017/10/18 15:04:11, juliandoucette wrote:
> On 2017/10/17 15:03:34, ire wrote:
> > On 2017/10/13 13:39:56, juliandoucette wrote:
> > > NIT: If you use the floating box model then you won't have to deal with
the
> > > extra whitespace. Especially on small screens (if this dropdown breaks to
> the
> > > next line).
> > 
> > Sorry, extra space where? When this moves onto its own line it should 100%
> width
> 
> No idea :/ . Doesn't seem to be an issue to me. Sorry.

No problem :)

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29573824/static/scss/compone...
static/scss/components/_select.scss:144: background-image:
url(/img/png/arrow-down-icon-secondary.png);
On 2017/10/18 15:04:11, juliandoucette wrote:
> On 2017/10/17 15:03:35, ire wrote:
> > On 2017/10/13 13:39:56, juliandoucette wrote:
> > > NIT: Can we use SVG or 2x PNG and fallback to 1x png somehow?
> > 
> > I know it's possible to set multiple background-image urls (e.g.
> > `background-image: url(one.png), url(two.png)`), but these will be overlaid
on
> > top of each other. I'm not sure what other methods there are to doing this.
> Will
> > look it up
> 
> https://css-tricks.com/svg-fallbacks/#article-header-id-4

Neat! Done.

[juliandoucette, 2017-10-23 13:40:07]

This doesn't seem to be hiding properly atm?

Also NIT it errors if an invalid browser name is loaded from localstorage.

[juliandoucette, 2017-10-23 13:43:12]

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl
File templates/minimal.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.t...
templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome">
I'm guessing that this is supposed to be uA-safari not uS-safari.

[ire, 2017-10-23 16:13:47]

On 2017/10/23 13:40:07, juliandoucette wrote:
> This doesn't seem to be hiding properly atm?

I haven't handled hiding/showing of content in this issue. I will do that
separately.

> Also NIT it errors if an invalid browser name is loaded from localstorage.

Why/how would an invalid browser name be loaded from localstorage? That would
only happen if the user goes in there themselves and messes around with things
(which I don't think we have to account for)

[ire, 2017-10-23 16:13:54]

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl
File templates/minimal.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.t...
templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome">
On 2017/10/23 13:43:12, juliandoucette wrote:
> I'm guessing that this is supposed to be uA-safari not uS-safari.

No I was testing to make sure that only ua-* classes where removed. All these
classes shouldn't be in the commit at all. Will remove in the next patchset

[ire, 2017-10-23 16:15:40]

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.tmpl
File templates/minimal.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29584600/templates/minimal.t...
templates/minimal.tmpl:46: <body class="us-safari ua-ios ua-safari ua-chrome">
On 2017/10/23 16:13:53, ire wrote:
> On 2017/10/23 13:43:12, juliandoucette wrote:
> > I'm guessing that this is supposed to be uA-safari not uS-safari.
> 
> No I was testing to make sure that only ua-* classes where removed. All these
> classes shouldn't be in the commit at all. Will remove in the next patchset

Done.

[juliandoucette, 2017-10-24 12:12:30]

All minor issues.

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
includes/browser-select.tmpl:1: 
Suggest: <fieldset>?

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
NIT: Missing alt text

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#n...
static/js/main.js:208: this.browserSelect = select;
NIT: You are repeating yourself by prefacing these names with "browser" e.g.
browserSelect.browserSelect browserSelect.browserCustomSelect vs
browserSelect.select browserSelect.customSelect

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:209: this.browserCustomSelect = new
CustomSelect(this.browserSelect);
NIT/Suggest: Looking at this again, I think that the browser select *is*, not
*has*, a CustomSelect. And it should extend the CustomSelect prototype for that
reason.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:221: var storedBrowser =
localStorage.getItem(this.BROWSER_STORAGE_KEY);
NIT/Suggest: Place this above event listeners with other definitions

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:222: if (storedBrowser)
NIT/Suggest: Our coding style permits braceless one line if/else blocks.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:230: }
NIT: Missing ";" (the same applies elsewhere)

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
NIT/Suggest:

this.supportedBrowsers = ["chorme", "firefox", ...]; // (should probably go in
the constructor)

for (var supportedBrowser in this.supportedBrowsers)
  if (browser[supportedBrowser])
    return this.selectOption(supportedBrowser);

localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
this.selectOption(this.DEFAULT_BROWSER);

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:259: // Save value to Local Storage
NIT: I don't think that this comment is necessary?

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:263: var bodyClassList =
Array.prototype.slice.call(document.body.classList);
document.body.classList has length. You don't need to make it into an Array. If
you don't, then you could use this variable below to bodyClassList.add &
bodyClassList.remove.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:274: var selectedItem = this.browserSelect
I think that we usually open the brace on the same line in this case (or the
next if we are chaining). The same applies below.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:279: var selectedOption = document.createElement("li");
You don't seem to be using this element (just manipulating it's innerHTML which
could be stored inside a variable instead of an element).

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
static/scss/components/_select.scss:108: 
NIT/Suggest: Move the padding left/right from the <ul> to the <li> so that you
can tab to focus the entire row instead of a ~button inside the row.

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
static/scss/components/_select.scss:131: padding-right: 0.7em;
NIT/TOL: It's strange that you are padding-right here but not padding-left
above.

https://codereview.adblockplus.org/29559620/diff/29586685/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29586685/templates/article.t...
templates/article.tmpl:17: <div class="article-browser-selector clearfix">
NIT: Why not @extend .clearfix?

[ire, 2017-10-27 10:23:00]

New patch ready

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
File includes/browser-select.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
includes/browser-select.tmpl:1: 
On 2017/10/24 12:12:27, juliandoucette wrote:
> Suggest: <fieldset>?

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/includes/browser-se...
includes/browser-select.tmpl:12: <img src="/img/png/logo-abp.png"
srcset="/img/svg/logo-abp.svg 2x">
On 2017/10/24 12:12:27, juliandoucette wrote:
> NIT: Missing alt text

Done.

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#n...
static/js/main.js:208: this.browserSelect = select;
On 2017/10/24 12:12:28, juliandoucette wrote:
> NIT: You are repeating yourself by prefacing these names with "browser" e.g.
> browserSelect.browserSelect browserSelect.browserCustomSelect vs
> browserSelect.select browserSelect.customSelect

You're right. Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:209: this.browserCustomSelect = new
CustomSelect(this.browserSelect);
On 2017/10/24 12:12:28, juliandoucette wrote:
> NIT/Suggest: Looking at this again, I think that the browser select *is*, not
> *has*, a CustomSelect. And it should extend the CustomSelect prototype for
that
> reason.

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:221: var storedBrowser =
localStorage.getItem(this.BROWSER_STORAGE_KEY);
On 2017/10/24 12:12:28, juliandoucette wrote:
> NIT/Suggest: Place this above event listeners with other definitions 

I think I prefer this being close to where it's used in this case.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:222: if (storedBrowser)
On 2017/10/24 12:12:29, juliandoucette wrote:
> NIT/Suggest: Our coding style permits braceless one line if/else blocks.

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:230: }
On 2017/10/24 12:12:28, juliandoucette wrote:
> NIT: Missing ";" (the same applies elsewhere)

The ";" isn't used for the closing brace of functions (function name() {}). 

But this applies elsewhere. Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/24 12:12:29, juliandoucette wrote:
> NIT/Suggest:
> 
> this.supportedBrowsers = ["chorme", "firefox", ...]; // (should probably go in
> the constructor)
> 
> for (var supportedBrowser in this.supportedBrowsers)
>   if (browser[supportedBrowser])
>     return this.selectOption(supportedBrowser);
> 
> localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
> this.selectOption(this.DEFAULT_BROWSER);

I need to check for the specific browser name to set it as the variable name we
want. This is why I didn't use an array.

The other option would be for us to use the same browser variable names as
bowser does.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:259: // Save value to Local Storage
On 2017/10/24 12:12:29, juliandoucette wrote:
> NIT: I don't think that this comment is necessary?

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:263: var bodyClassList =
Array.prototype.slice.call(document.body.classList);
On 2017/10/24 12:12:27, juliandoucette wrote:
> document.body.classList has length. You don't need to make it into an Array.
If
> you don't, then you could use this variable below to bodyClassList.add &
> bodyClassList.remove.

I can't do it that way because when a class is removed, I can't rely on the
index again to determine where it is. I have to create a separate array that
doesn't change it's length.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:274: var selectedItem = this.browserSelect
On 2017/10/24 12:12:29, juliandoucette wrote:
> I think that we usually open the brace on the same line in this case (or the
> next if we are chaining). The same applies below.

I moved the .querySelector line because it would be longer than 80 characters.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:279: var selectedOption = document.createElement("li");
On 2017/10/24 12:12:28, juliandoucette wrote:
> You don't seem to be using this element (just manipulating it's innerHTML
which
> could be stored inside a variable instead of an element).

You're right, don't notice that :p

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
File static/scss/components/_select.scss (right):

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
static/scss/components/_select.scss:108: 
On 2017/10/24 12:12:29, juliandoucette wrote:
> NIT/Suggest: Move the padding left/right from the <ul> to the <li> so that you
> can tab to focus the entire row instead of a ~button inside the row.

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/static/scss/compone...
static/scss/components/_select.scss:131: padding-right: 0.7em;
On 2017/10/24 12:12:29, juliandoucette wrote:
> NIT/TOL: It's strange that you are padding-right here but not padding-left
> above. 

Done.

https://codereview.adblockplus.org/29559620/diff/29586685/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29586685/templates/article.t...
templates/article.tmpl:17: <div class="article-browser-selector clearfix">
On 2017/10/24 12:12:30, juliandoucette wrote:
> NIT: Why not @extend .clearfix?

Done.

[juliandoucette, 2017-10-31 13:20:00]

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#n...
static/js/main.js:230: }
On 2017/10/27 10:22:57, ire wrote:
> On 2017/10/24 12:12:28, juliandoucette wrote:
> > NIT: Missing ";" (the same applies elsewhere)
> 
> The ";" isn't used for the closing brace of functions (function name() {}). 
> 
> But this applies elsewhere. Done.

*facepalm* (because I commented on the only place where my comment didn't apply)

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/27 10:22:57, ire wrote:
> I need to check for the specific browser name to set it as the variable name
we
> want. This is why I didn't use an array.

I did so by setting the variable `supportedBrowser` in my example. I don't see
any other relevant usages of your overridden `browser` variable?

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:263: var bodyClassList =
Array.prototype.slice.call(document.body.classList);
On 2017/10/27 10:22:57, ire wrote:
> On 2017/10/24 12:12:27, juliandoucette wrote:
> > document.body.classList has length. You don't need to make it into an Array.
> If
> > you don't, then you could use this variable below to bodyClassList.add &
> > bodyClassList.remove.
> 
> I can't do it that way because when a class is removed, I can't rely on the
> index again to determine where it is. I have to create a separate array that
> doesn't change it's length.

Acknowledged. Doh.

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:274: var selectedItem = this.browserSelect
On 2017/10/27 10:22:58, ire wrote:
> On 2017/10/24 12:12:29, juliandoucette wrote:
> > I think that we usually open the brace on the same line in this case (or the
> > next if we are chaining). The same applies below.
> 
> I moved the .querySelector line because it would be longer than 80 characters.

I know. My comment still applies? We usually open the brace instead. And if we
don't then we start one indentation after the start of the line. This isn't
spelled out in our coding style; unfortunately. And it seems like a common case.
Maybe we can find or add something about it.

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:39: console.log(select)
Hello, world

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:241: this.BROWSER_STORAGE_KEY = "BROWSER";
NIT/Suggest: The word "STORAGE" is unnecessary (very opinionated - ignore if you
disagree)

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:255: BrowserSelect.prototype = CustomSelect.prototype;
This assignment is by reference; not by copy. As a result, you are
unintentionally extending CustomSelect.prototype below. I suggest using
Object.create @see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[juliandoucette, 2017-10-31 13:24:03]

@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#n...
static/js/main.js:274: var selectedItem = this.browserSelect
On 2017/10/31 13:19:58, juliandoucette wrote:
> On 2017/10/27 10:22:58, ire wrote:
> > On 2017/10/24 12:12:29, juliandoucette wrote:
> > > I think that we usually open the brace on the same line in this case (or
the
> > > next if we are chaining). The same applies below.
> > 
> > I moved the .querySelector line because it would be longer than 80
characters.
> 
> I know. My comment still applies? We usually open the brace instead. And if we
> don't then we start one indentation after the start of the line. This isn't
> spelled out in our coding style; unfortunately. And it seems like a common
case.
> Maybe we can find or add something about it.

Can you advise us on this kzar?

[kzar, 2017-10-31 14:22:32]

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#n...
static/js/main.js:274: var selectedItem = this.browserSelect
On 2017/10/31 13:24:02, juliandoucette wrote:
> On 2017/10/31 13:19:58, juliandoucette wrote:
> > On 2017/10/27 10:22:58, ire wrote:
> > > On 2017/10/24 12:12:29, juliandoucette wrote:
> > > > I think that we usually open the brace on the same line in this case (or
> the
> > > > next if we are chaining). The same applies below.
> > > 
> > > I moved the .querySelector line because it would be longer than 80
> characters.
> > 
> > I know. My comment still applies? We usually open the brace instead. And if
we
> > don't then we start one indentation after the start of the line. This isn't
> > spelled out in our coding style; unfortunately. And it seems like a common
> case.
> > Maybe we can find or add something about it.
> 
> Can you advise us on this kzar?

IIRC we don't have a ESLint rule for this case, so it's up to you as long as the
code lints. I do remember seeing Wladimir do something similar recently[1].

P.S. if it helps I found a bunch of lines starting with a dot operator[2], have
a look through those if you want to have a look at how it's commonly done.

[1]
https://github.com/adblockplus/adblockplusui/commit/f5403db7476aadd1c7519485a...
[2] https://gist.github.com/kzar/8d3e7c6ff8c7f02e776b87ffd9f63f14

[ire, 2017-11-01 12:36:50]

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#n...
static/js/main.js:230: }
On 2017/10/31 13:19:57, juliandoucette wrote:
> On 2017/10/27 10:22:57, ire wrote:
> > On 2017/10/24 12:12:28, juliandoucette wrote:
> > > NIT: Missing ";" (the same applies elsewhere)
> > 
> > The ";" isn't used for the closing brace of functions (function name() {}). 
> > 
> > But this applies elsewhere. Done.
> 
> *facepalm* (because I commented on the only place where my comment didn't
apply)

Haha :D

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/10/31 13:19:58, juliandoucette wrote:
> On 2017/10/27 10:22:57, ire wrote:
> > I need to check for the specific browser name to set it as the variable name
> we
> > want. This is why I didn't use an array.
> 
> I did so by setting the variable `supportedBrowser` in my example. I don't see
> any other relevant usages of your overridden `browser` variable?

Ooo I see! I don't think I read your example properly, sorry about that.

The only other thing is that we aren't using the same browser names as bowser
does. So we can't set the browser to be "yandexBrowser" because we would want it
to be "yandex". So our supportedBrowsers array will actually have to container
two variables (bowser name and our name). It still think your method would be
cleaner, but may not necessarily be shorter. What do you think?

https://codereview.adblockplus.org/29559620/diff/29586685/static/js/main.js#n...
static/js/main.js:274: var selectedItem = this.browserSelect
On 2017/10/31 14:22:31, kzar wrote:
> On 2017/10/31 13:24:02, juliandoucette wrote:
> > On 2017/10/31 13:19:58, juliandoucette wrote:
> > > On 2017/10/27 10:22:58, ire wrote:
> > > > On 2017/10/24 12:12:29, juliandoucette wrote:
> > > > > I think that we usually open the brace on the same line in this case
(or
> > the
> > > > > next if we are chaining). The same applies below.
> > > > 
> > > > I moved the .querySelector line because it would be longer than 80
> > characters.
> > > 
> > > I know. My comment still applies? We usually open the brace instead. And
if
> we
> > > don't then we start one indentation after the start of the line. This
isn't
> > > spelled out in our coding style; unfortunately. And it seems like a common
> > case.
> > > Maybe we can find or add something about it.
> > 
> > Can you advise us on this kzar?
> 
> IIRC we don't have a ESLint rule for this case, so it's up to you as long as
the
> code lints. I do remember seeing Wladimir do something similar recently[1].
> 
> P.S. if it helps I found a bunch of lines starting with a dot operator[2],
have
> a look through those if you want to have a look at how it's commonly done.
> 
> [1]
>
https://github.com/adblockplus/adblockplusui/commit/f5403db7476aadd1c7519485a...
> [2] https://gist.github.com/kzar/8d3e7c6ff8c7f02e776b87ffd9f63f14

Thanks kzar!

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:39: console.log(select)
On 2017/10/31 13:19:58, juliandoucette wrote:
> Hello, world

Oops! Thanks :)

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:241: this.BROWSER_STORAGE_KEY = "BROWSER";
On 2017/10/31 13:19:59, juliandoucette wrote:
> NIT/Suggest: The word "STORAGE" is unnecessary (very opinionated - ignore if
you
> disagree)

I don't think it's unnecessary, because it's related to the "storage key", not
just the "browser key" if that makes sense. If anything, the word "BROWSER"
might be what is unnecessary, but I'd rather leave it for clarity.

https://codereview.adblockplus.org/29559620/diff/29590562/static/js/main.js#n...
static/js/main.js:255: BrowserSelect.prototype = CustomSelect.prototype;
On 2017/10/31 13:19:59, juliandoucette wrote:
> This assignment is by reference; not by copy. As a result, you are
> unintentionally extending CustomSelect.prototype below. I suggest using
> Object.create @see
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

Thanks! Done.

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js#n...
static/js/main.js:298: .querySelector("[data-value='" + browser + "']");
@julian Is this what you mean?

[juliandoucette, 2017-11-01 12:49:22]

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#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/11/01 12:36:47, ire wrote:
> On 2017/10/31 13:19:58, juliandoucette wrote:
> > On 2017/10/27 10:22:57, ire wrote:
> > > I need to check for the specific browser name to set it as the variable
name
> > we
> > > want. This is why I didn't use an array.
> > 
> > I did so by setting the variable `supportedBrowser` in my example. I don't
see
> > any other relevant usages of your overridden `browser` variable?
> 
> Ooo I see! I don't think I read your example properly, sorry about that.
> 
> The only other thing is that we aren't using the same browser names as bowser
> does. So we can't set the browser to be "yandexBrowser" because we would want
it
> to be "yandex". So our supportedBrowsers array will actually have to container
> two variables (bowser name and our name). It still think your method would be
> cleaner, but may not necessarily be shorter. What do you think?

Why don't we use their names instead?

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js#n...
static/js/main.js:298: .querySelector("[data-value='" + browser + "']");
On 2017/11/01 12:36:49, ire wrote:
> @julian Is this what you mean?

Yes.

RE: kzar's comment.

I think kzar debunked my claim and we are actually inconsistent about how we do
this across repositories. I'm OK with whatever.

[ire, 2017-11-01 17:20:13]

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#n...
static/js/main.js:237: if (bowser.chrome) browser = "chrome";
On 2017/11/01 12:49:21, juliandoucette wrote:
> On 2017/11/01 12:36:47, ire wrote:
> > On 2017/10/31 13:19:58, juliandoucette wrote:
> > > On 2017/10/27 10:22:57, ire wrote:
> > > > I need to check for the specific browser name to set it as the variable
> name
> > > we
> > > > want. This is why I didn't use an array.
> > > 
> > > I did so by setting the variable `supportedBrowser` in my example. I don't
> see
> > > any other relevant usages of your overridden `browser` variable?
> > 
> > Ooo I see! I don't think I read your example properly, sorry about that.
> > 
> > The only other thing is that we aren't using the same browser names as
bowser
> > does. So we can't set the browser to be "yandexBrowser" because we would
want
> it
> > to be "yandex". So our supportedBrowsers array will actually have to
container
> > two variables (bowser name and our name). It still think your method would
be
> > cleaner, but may not necessarily be shorter. What do you think?
> 
> Why don't we use their names instead?

We could. Done.

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29559620/diff/29594585/static/js/main.js#n...
static/js/main.js:298: .querySelector("[data-value='" + browser + "']");
On 2017/11/01 12:49:21, juliandoucette wrote:
> On 2017/11/01 12:36:49, ire wrote:
> > @julian Is this what you mean?
> 
> Yes.
> 
> RE: kzar's comment.
> 
> I think kzar debunked my claim and we are actually inconsistent about how we
do
> this across repositories. I'm OK with whatever.

Okay, I prefer my way then :p

[juliandoucette, 2017-11-02 13:10:57]

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.t...
templates/article.tmpl:56: <script src="/js/vendor/bowser.js"></script>
suggest: We could add scripts like these to a scripts block

[ire, 2017-11-02 14:31:05]

On 2017/11/02 13:10:57, juliandoucette wrote:
> I'm seeing "[Browser selector]" on article pages.

Oops. Fixed.

[ire, 2017-11-02 14:31:15]

https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.tmpl
File templates/article.tmpl (right):

https://codereview.adblockplus.org/29559620/diff/29594638/templates/article.t...
templates/article.tmpl:56: <script src="/js/vendor/bowser.js"></script>
On 2017/11/02 13:10:56, juliandoucette wrote:
> suggest: We could add scripts like these to a scripts block

Good idea. Done.

I think it might also be a good idea to have a separate file for the article
page. There's probably not much point loading the js for the browser-selector on
other pages for example. I'll handle that separately if you agree.

[juliandoucette, 2017-11-02 15:22:21]

LGTM + Suggestions

Suggestions may be addressed separately or ignored.

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
suggest: change background-color on hove/active/focus of select and options

https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/compone...
static/scss/components/_select.scss:111: // Custom Dropdown (extends the custom
select element)
suggest: category comment (underline it)

https://codereview.adblockplus.org/29559620/diff/29595601/static/scss/compone...
static/scss/components/_select.scss:143: border-left: 2px solid $gray-medium;
suggest: 1px lighter gray throughout

[ire, 2017-11-03 08:45:37]

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