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

Issue 29626565: Issue 6115 - Refactored icon classes (Closed)

Created:
Dec. 1, 2017, 6:33 p.m. by saroyanm
Modified:
Dec. 12, 2017, 10:56 a.m.
Reviewers:
ire
CC:
Thomas Greiner
Visibility:
Public.

Description

Issue 6115 - Refactored icon classes

Patch Set 1 : #

Total comments: 12

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -268 lines) Patch
M desktop-options.html View 13 chunks +25 lines, -25 lines 0 comments Download
M desktop-options.js View 1 chunk +1 line, -1 line 0 comments Download
M skin/desktop-options.css View 1 19 chunks +235 lines, -242 lines 0 comments Download

Messages

Total messages: 7
saroyanm
@Ire can you please have a look if you have a chance ? https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css File ...
Dec. 1, 2017, 6:44 p.m. (2017-12-01 18:44:46 UTC) #1
ire
Thanks Manvel, just a few comments below. https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css#newcode155 skin/desktop-options.css:155: button.primary:not([disabled]):not(.icon):hover, NIT: ...
Dec. 4, 2017, 1:26 p.m. (2017-12-04 13:26:12 UTC) #2
saroyanm
https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css#newcode155 skin/desktop-options.css:155: button.primary:not([disabled]):not(.icon):hover, On 2017/12/04 13:26:12, ire wrote: > NIT: These ...
Dec. 5, 2017, 1:47 p.m. (2017-12-05 13:47:57 UTC) #3
ire
https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css#newcode155 skin/desktop-options.css:155: button.primary:not([disabled]):not(.icon):hover, On 2017/12/05 13:47:57, saroyanm wrote: > On 2017/12/04 ...
Dec. 7, 2017, 11:28 a.m. (2017-12-07 11:28:18 UTC) #4
saroyanm
Sorry Ire, for updating the patch so late, I've got busy with another release preparation ...
Dec. 11, 2017, 12:23 p.m. (2017-12-11 12:23:50 UTC) #5
ire
Thanks Manvel. LGTM https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css File skin/desktop-options.css (right): https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-options.css#newcode155 skin/desktop-options.css:155: button.primary:not([disabled]):not(.icon):hover, On 2017/12/11 12:23:49, saroyanm wrote: ...
Dec. 12, 2017, 9:45 a.m. (2017-12-12 09:45:34 UTC) #6
saroyanm
Dec. 12, 2017, 10:56 a.m. (2017-12-12 10:56:10 UTC) #7
https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option...
File skin/desktop-options.css (right):

https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option...
skin/desktop-options.css:155: button.primary:not([disabled]):not(.icon):hover,
On 2017/12/12 09:45:33, ire wrote:
> On 2017/12/11 12:23:49, saroyanm wrote:
> > On 2017/12/07 11:28:18, ire wrote:
> > > On 2017/12/05 13:47:57, saroyanm wrote:
> > > > On 2017/12/04 13:26:12, ire wrote:
> > > > > NIT: These selectors are starting to get really long. I would suggest
> just
> > > > > overriding these classes below rather than having this long selector
to
> > add
> > > > the
> > > > > `:not(.icon)`. (From my test, just removing this additional qualifier
> > > doesn't
> > > > > actually change anything.)
> > > > I think the reason I did this is separation:
> > > > 
> > > > With current implementation we do have couple of buttons: Regular
buttons
> > that
> > > > look like buttons and buttons that look like "icons", "primary",
> "secondary"
> > > and
> > > > "tertiary" provide additional styles to them which are different for
both
> of
> > > > them (example hover on close "prymary" icon).
> > > > 
> > > > In order to avoid :not(.icon) here and overriding styles in multiple
> places
> > I
> > > > think I need to add another class to regular button in order to separate
> it.
> > > > ex.: "regular", "default" or "basic". I think smth among those lines. I
> like
> > > > regular most.
> > > 
> > > I think you may have misunderstood what I was saying. If you remove the
> > > `:not(.icon)` part of this selector, none of the existing buttons (that
> aren't
> > > icons) will be affected. The only styles you will need to override are for
> the
> > > .icon buttons specifically.
> > I would like to avoid overriding styles if possible, overriding styles makes
> > properties introduction more error prone in this case, for example if we
> > introduce a new property here we will need to override that property in
> > .icon.primary:hover class and it's easy to miss IMO.
> > 
> > > Note: This comment is more for the readability, because I think this
section
> > > styling the buttons is starting to look a bit confusing. Adding comments
may
> > > help, but that's probably for another issue/time.
> > I think we should add a comment here. To describe the reason of ignoring the
> > ".icon"
> > 
> > Please let me know Ire if you think that it's still hard to read this part
> with
> > the new patch, maybe we can think of another way of dealing with "specific"
> > styles and also avoid overriding them in multiple places if possible.
> 
> Yes this reads better now. 
> 
> Since this is such a long file, my suggestion was more for how to use comments
> to better organise the whole file (I think the way we have been using comments
> in website defaults would be helpful). But like I mentioned, that's not
relevant
> to this particular issue. 

I agree comments and modularization should make things more readable, I think
both can be tackled as part of -> https://issues.adblockplus.org/ticket/6117

Powered by Google App Engine
This is Rietveld