Issue 5628 - Implement main styles

[saroyanm, 2017-09-06 19:02:57]

This is ready for the review.

Julian in case you and Ire will decide to switch, so Ire will review the code,
depending who will be able more active reviewing this, please let me know so
I'll change.

Otherwise I made comments regarding questionable parts.
If there are enhancements that need to be done on JS side, I'll create another
issue for that, not to complicate the merging process.

As a side note: I'm taking day offs on Friday and Monday, so I'll be able to get
my hands on this on Tuesday. In case urgency/input needed will try comment over
the Dayoffs/weekend, but hope it will not be required.

Looking forward to see this finished, as soon we will be able to push this as
much things we will be able to fix/enhance before the deadline.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:120: <div id="blocking-languages">
Note: There is a ticket for improving semantics using <forms> and Submit button.
See -> https://issues.adblockplus.org/ticket/5251

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:205: -webkit-mask: url(icons/trash.svg);
Note: I'm using masks currently as it's the most quick and easy way to change
the color on different states. In general this will be changed and be consistent
with the implementation of the toggle.svg. See ->
https://bitbucket.org/adblockplus/spec/issues/73

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:998: border-right: 10px solid transparent;
Note: I used this solution until we will have asset for the "tooltip arrow", see
-> https://bitbucket.org/adblockplus/spec/issues/71 

Also we have another ticket where we will finalize and improve the accessibility
of Tooltips, see -> https://issues.adblockplus.org/ticket/5542

[juliandoucette, 2017-09-11 22:25:06]

I think we'd better extreme review this one (review in the office or via
bluejeans). Otherwise I'll have several comments (most of which are probably
unnecessary) on every line. I'll clear my schedule whenever you're ready if you
agree.

[juliandoucette, 2017-09-12 22:45:39]

Mostly NITs, Notes, and questions.

As we (Manvel and I) discussed, there are many inconsistencies between the spec,
the styleguide, and this implementation. And these inconsistencies are mostly
unimportant / probably better identified and addressed separately (according to
their priority). I think it's important that we get this into testing so that we
can address functional issues ASAP and defer functional improvements and
refactoring until before / after launch (according to their priority).

On that note, I don't think that I have found anything blocking in this review.
Please check the notes and questions below and then proceed to push to testing
and address these comments in this review or separately as needed.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:79: <h1 class="i18n_options_tab_general"></h1>
NIT/Suggest: Wrap <h1> and <p> inside a <header>

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:85: <button data-action="toggle-remove-subscription"
role="checkbox" class="control"></button>
NIT/Suggest: Implement a custom checkbox like this
https://getbootstrap.com/docs/4.0/components/forms/#checkboxes-and-radios-1 and
place input inside label so that clicking the label selects the input.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:120: <div id="blocking-languages">
On 2017/09/06 19:02:56, saroyanm wrote:
> Note: There is a ticket for improving semantics using <forms> and Submit
button.
> See -> https://issues.adblockplus.org/ticket/5251

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:138: <ul id="custom-list-table" class="table list">
NIT: I think "table" and "list" is redundant here.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:174: <h1 class="i18n_options_tab_advanced"></h1>
NIT/Suggest: Wrap this <h1> and <p> in a <header>

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:203: <div id="all-filter-lists-table-header" class="th">
Note: It's not clear why you didn't use a real table here. (No big deal.)

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:209: <ul class="table cols" id="all-filter-lists-table">
Note: 

I was able to:

- Make this list unresponsive
- Add empty items to this list
- Cause clicking one switch in this list to activate another

by playing with the controls available in this UI.

It's not clear whether or not these are bugs that you should address in this
patchset. I suspect that they are not.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:212: <button data-action="toggle-disable-subscription"
role="checkbox" class="control toggle">
NIT: This switch shows 1px from the left on the right (at least on highDPI
screens) (like when you show a sprite in a spritesheet and you see 1px of the
next sprite in the grid)

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:212: <button data-action="toggle-disable-subscription"
role="checkbox" class="control toggle">
NIT/Suggest: A checkbox is a more appropriate input for this purpose.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:262: <p class="i18n_options_customFilters_tip"></p>
NIT: You're missing an icon here.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:263: <button class="i18n_options_customFilters_start secondary"
data-action="edit-custom-filters">
NIT: This is supposed to be a primary button.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:266: <a class="i18n_options_customFilters_learn"
id="link-filters" target="_blank"></a>
NIT: This string is missing a period.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:334: <p class="i18n_options_dialog_about_copyright"></p>
NIT: This should probably be two paragraphs (based on the spec)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:59: font-weight: 400;
NIT: I think 400 is the default across browsers.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:205: -webkit-mask: url(icons/trash.svg);
On 2017/09/06 19:02:56, saroyanm wrote:
> Note: I'm using masks currently as it's the most quick and easy way to change
> the color on different states. In general this will be changed and be
consistent
> with the implementation of the toggle.svg. See ->
> https://bitbucket.org/adblockplus/spec/issues/73

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:215: button.gear,
NIT/Suggest: button.icon

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:420: main > div
NIT: This could use some explanation via comment (in the future)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:441: [role="tabpanel"] > h1,
Note: You would have to update these selectors accordingly if you wraped these
in a <header> like I suggested.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:531: ul.table,
Note: See related comment in new-options.html

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:581: .table.list li.empty-placeholder
I wasn't able to find this in the HTML. Does it only appear when some action is
taking place?

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:591: #blocking-languages-table .display
NIT: Why the name "display"?

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:602: .table.list button.link:hover
NIT/Suggest: Make this less specific (button.link:hover) and move it up by
a:hover

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:668: .table.cols .display
NIT: See comment above RE: display

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:680: -moz-margin-start: 1.1rem;
NIT: Vendor prefixes? (applies elsewhere too)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:684: .table.cols .gear
NIT: This doesn't seem to make a difference in Firefox or Chrome?
NIT: It's unclear why this would apply to .gear and not .delete too?

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:732: #dialog .table.list li button .display
NIT: See comment above about .display

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:743: .tooltip
NIT: It's confusing that you call this tooltip. It's not the tooltip itself.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:752: text-decoration: none;
NIT: I wasn't able to find where text-decoration: none was necessary

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:753: cursor: default;
NIT/Suggest: cursor: help (obviously)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:866: #custom-filters-raw ~ button,
NIT: Why not just #custom-filters button?

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:874: #custom-filters h3
NIT: It seems like this could fit into section h3, .section h3 above (or
something like it)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:883: padding: 0.8rem 1.3rem;
Note: I think the difference between x and y here is a styleguide bug (TBD
separately)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:892: #custom-filters-raw-controls
NIT: Looks like you're missing some margin-top here.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:931: width: 15rem;
NIT: It's not clear where this width comes from

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:998: border-right: 10px solid transparent;
On 2017/09/06 19:02:57, saroyanm wrote:
> Note: I used this solution until we will have asset for the "tooltip arrow",
see
> -> https://bitbucket.org/adblockplus/spec/issues/71 
> 
> Also we have another ticket where we will finalize and improve the
accessibility
> of Tooltips, see -> https://issues.adblockplus.org/ticket/5542

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1192: #dialog-close,
NIT/Suggest: This could have something in common with an .icon class.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1202: #dialog-close::before,
NIT: Why you are adding this background image ::before the button instead of to
the button?

(The same applies elsewhere.)

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1267: border-style: solid;;
NIT: The default border-style is solid? And you have two ";" here.

[saroyanm, 2017-09-13 15:26:39]

Notes from the meeting.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:79: <h1 class="i18n_options_tab_general"></h1>
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT/Suggest: Wrap <h1> and <p> inside a <header>

Agree.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:85: <button data-action="toggle-remove-subscription"
role="checkbox" class="control"></button>
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT/Suggest: Implement a custom checkbox like this
> https://getbootstrap.com/docs/4.0/components/forms/#checkboxes-and-radios-1
and
> place input inside label so that clicking the label selects the input.

We will file another issue to address separately.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:138: <ul id="custom-list-table" class="table list">
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT: I think "table" and "list" is redundant here.

I'm using .table for the "list items" that has border around them which applies
to the child <li> elements.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:174: <h1 class="i18n_options_tab_advanced"></h1>
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT/Suggest: Wrap this <h1> and <p> in a <header>

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:203: <div id="all-filter-lists-table-header" class="th">
On 2017/09/12 22:45:35, juliandoucette wrote:
> Note: It's not clear why you didn't use a real table here. (No big deal.)

We have issue to address that -> https://issues.adblockplus.org/ticket/5317

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:209: <ul class="table cols" id="all-filter-lists-table">
On 2017/09/12 22:45:34, juliandoucette wrote:
> Note: 
> 
> I was able to:
> 
> - Make this list unresponsive
> - Add empty items to this list
> - Cause clicking one switch in this list to activate another
Most of them are caused by adding empty filter list and they should be fixed in
-> https://issues.adblockplus.org/ticket/5549

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:212: <button data-action="toggle-disable-subscription"
role="checkbox" class="control toggle">
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT/Suggest: A checkbox is a more appropriate input for this purpose.

Same as mentioned above.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:212: <button data-action="toggle-disable-subscription"
role="checkbox" class="control toggle">
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT: This switch shows 1px from the left on the right (at least on highDPI
> screens) (like when you show a sprite in a spritesheet and you see 1px of the
> next sprite in the grid)

I'll update width of icon to -> 2.25rem

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:262: <p class="i18n_options_customFilters_tip"></p>
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT: You're missing an icon here.

Yes, we are waiting for the assets

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:263: <button class="i18n_options_customFilters_start secondary"
data-action="edit-custom-filters">
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT: This is supposed to be a primary button.

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:266: <a class="i18n_options_customFilters_learn"
id="link-filters" target="_blank"></a>
On 2017/09/12 22:45:34, juliandoucette wrote:
> NIT: This string is missing a period.

In specs documentation we are not using period.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:334: <p class="i18n_options_dialog_about_copyright"></p>
On 2017/09/12 22:45:35, juliandoucette wrote:
> NIT: This should probably be two paragraphs (based on the spec)

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:59: font-weight: 400;
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: I think 400 is the default across browsers.

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:215: button.gear,
On 2017/09/12 22:45:38, juliandoucette wrote:
> NIT/Suggest: button.icon

We already specified .icon which loads png sprite, we will use common icon
styles after refactoring.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:441: [role="tabpanel"] > h1,
On 2017/09/12 22:45:37, juliandoucette wrote:
> Note: You would have to update these selectors accordingly if you wraped these
> in a <header> like I suggested.

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:531: ul.table,
On 2017/09/12 22:45:38, juliandoucette wrote:
> Note: See related comment in new-options.html

See comment on the other file.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:581: .table.list li.empty-placeholder
On 2017/09/12 22:45:36, juliandoucette wrote:
> I wasn't able to find this in the HTML. Does it only appear when some action
is
> taking place?

We are adding the placeholder when the table is empty

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:591: #blocking-languages-table .display
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: Why the name "display"?

We might revisit if we will only use this for filter list titles.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:602: .table.list button.link:hover
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT/Suggest: Make this less specific (button.link:hover) and move it up by
> a:hover

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:680: -moz-margin-start: 1.1rem;
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT: Vendor prefixes? (applies elsewhere too)

This probably make sense to address separately as all the browsers we are
targeting in this review supporting them, but we should include the default
implementation without prefixes if one exists.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:684: .table.cols .gear
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT: This doesn't seem to make a difference in Firefox or Chrome?
> NIT: It's unclear why this would apply to .gear and not .delete too?

I think this style need to be specific to the "icon" style in general which will
be addressed after getting rid of current PNG sprite.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:743: .tooltip
On 2017/09/12 22:45:38, juliandoucette wrote:
> NIT: It's confusing that you call this tooltip. It's not the tooltip itself.

tooltip is within the the "tooltip" container, I'll address icon accessibility
separately.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:752: text-decoration: none;
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT: I wasn't able to find where text-decoration: none was necessary

We probably have link inside of the tooltip.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:753: cursor: default;
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT/Suggest: cursor: help (obviously)

We will revisit that separately after the style guide is updated regarding the
cursors.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:866: #custom-filters-raw ~ button,
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: Why not just #custom-filters button?

I'll wrap the buttons.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:874: #custom-filters h3
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: It seems like this could fit into section h3, .section h3 above (or
> something like it)

We need to target this specifically because of the table headings.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:883: padding: 0.8rem 1.3rem;
On 2017/09/12 22:45:37, juliandoucette wrote:
> Note: I think the difference between x and y here is a styleguide bug (TBD
> separately)

Style guide error.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:892: #custom-filters-raw-controls
On 2017/09/12 22:45:37, juliandoucette wrote:
> NIT: Looks like you're missing some margin-top here.

Acknowledged.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:931: width: 15rem;
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: It's not clear where this width comes from

eyepick

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1202: #dialog-close::before,
On 2017/09/12 22:45:38, juliandoucette wrote:
> NIT: Why you are adding this background image ::before the button instead of
to
> the button?
> 
> (The same applies elsewhere.)

Acknowledged. I'll address this separately.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1267: border-style: solid;;
On 2017/09/12 22:45:36, juliandoucette wrote:
> NIT: The default border-style is solid? And you have two ";" here.

Acknowledged.

[saroyanm, 2017-09-13 16:47:59]

New patch uploaded, ready for the review.
Should be easy one I think, because most of the issues are planned to address
separately as mentioned in the comments.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html
File new-options.html (right):

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:79: <h1 class="i18n_options_tab_general"></h1>
On 2017/09/13 15:26:35, saroyanm wrote:
> On 2017/09/12 22:45:36, juliandoucette wrote:
> > NIT/Suggest: Wrap <h1> and <p> inside a <header>
> 
> Agree.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:212: <button data-action="toggle-disable-subscription"
role="checkbox" class="control toggle">
On 2017/09/13 15:26:35, saroyanm wrote:
> On 2017/09/12 22:45:35, juliandoucette wrote:
> > NIT: This switch shows 1px from the left on the right (at least on highDPI
> > screens) (like when you show a sprite in a spritesheet and you see 1px of
the
> > next sprite in the grid)
> 
> I'll update width of icon to -> 2.25rem

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:263: <button class="i18n_options_customFilters_start secondary"
data-action="edit-custom-filters">
On 2017/09/13 15:26:36, saroyanm wrote:
> On 2017/09/12 22:45:35, juliandoucette wrote:
> > NIT: This is supposed to be a primary button.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/new-options.html#ne...
new-options.html:334: <p class="i18n_options_dialog_about_copyright"></p>
On 2017/09/13 15:26:35, saroyanm wrote:
> On 2017/09/12 22:45:35, juliandoucette wrote:
> > NIT: This should probably be two paragraphs (based on the spec)
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:59: font-weight: 400;
On 2017/09/13 15:26:37, saroyanm wrote:
> On 2017/09/12 22:45:36, juliandoucette wrote:
> > NIT: I think 400 is the default across browsers.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:441: [role="tabpanel"] > h1,
On 2017/09/13 15:26:36, saroyanm wrote:
> On 2017/09/12 22:45:37, juliandoucette wrote:
> > Note: You would have to update these selectors accordingly if you wraped
these
> > in a <header> like I suggested.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:602: .table.list button.link:hover
On 2017/09/13 15:26:36, saroyanm wrote:
> On 2017/09/12 22:45:37, juliandoucette wrote:
> > NIT/Suggest: Make this less specific (button.link:hover) and move it up by
> > a:hover
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:866: #custom-filters-raw ~ button,
On 2017/09/13 15:26:37, saroyanm wrote:
> On 2017/09/12 22:45:36, juliandoucette wrote:
> > NIT: Why not just #custom-filters button?
> 
> I'll wrap the buttons.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:892: #custom-filters-raw-controls
On 2017/09/13 15:26:36, saroyanm wrote:
> On 2017/09/12 22:45:37, juliandoucette wrote:
> > NIT: Looks like you're missing some margin-top here.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29536681/diff/29537671/skin/new-options.cs...
skin/new-options.css:1267: border-style: solid;;
On 2017/09/13 15:26:36, saroyanm wrote:
> On 2017/09/12 22:45:36, juliandoucette wrote:
> > NIT: The default border-style is solid? And you have two ";" here.
> 
> Acknowledged.

Done.

[juliandoucette, 2017-09-13 17:12:50]

LGTM + 1 NIT (In addition to those that we documented to address separately.)

https://codereview.adblockplus.org/29536681/diff/29543759/skin/new-options.css
File skin/new-options.css (right):

https://codereview.adblockplus.org/29536681/diff/29543759/skin/new-options.cs...
skin/new-options.css:864: .side-control button
NIT/Suggest: .side-controls (plural)

[saroyanm, 2017-09-13 17:21:45]

Thanks a lot, fixed the nit.