Issue 29626565: Issue 6115 - Refactored icon classes (Closed)
DescriptionIssue 6115 - Refactored icon classes
Patch Set 1 : #
Total comments: 12
Patch Set 2 : #
MessagesTotal messages: 7
@Ire can you please have a look if you have a chance ? 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:330: } Maybe we can move all background images and width-heights for icons here ? To make it more easy manageable ?
Thanks Manvel, just a few comments below. 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, 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 realise this is a personal preference so please go with whichever way you prefer) https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option... skin/desktop-options.css:179: button[role="checkbox"].icon::before NIT: It looks like there are never any `button[role="checkbox"]` that aren't also `.icon`. I don't think the extra `.icon` modifier is necessary (although maybe it makes it clearer that this is an icon?). If you move this style to the icons group like you mentioned below, then maybe that will be sufficient. https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option... skin/desktop-options.css:278: border-style: solid;; Noticed an extra ";" at the end of this line https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option... skin/desktop-options.css:330: } On 2017/12/01 18:44:46, saroyanm wrote: > Maybe we can move all background images and width-heights for icons here ? > To make it more easy manageable ? I agree
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/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 realise this is a personal preference so please go with whichever way you > prefer) https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option... skin/desktop-options.css:179: button[role="checkbox"].icon::before On 2017/12/04 13:26:12, ire wrote: > NIT: It looks like there are never any `button[role="checkbox"]` that aren't > also `.icon`. I don't think the extra `.icon` modifier is necessary (although > maybe it makes it clearer that this is an icon?). If you move this style to the > icons group like you mentioned below, then maybe that will be sufficient. Good point. https://codereview.adblockplus.org/29626565/diff/29626570/skin/desktop-option... skin/desktop-options.css:278: border-style: solid;; On 2017/12/04 13:26:12, ire wrote: > Noticed an extra ";" at the end of this line well spotted.
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/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. 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.
Sorry Ire, for updating the patch so late, I've got busy with another release preparation last week. New patch uploaded. 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/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.
Thanks Manvel. LGTM 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/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.
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 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
