Fix #3409 - Sass Linting Errors #3751
Conversation
| @@ -18,39 +18,39 @@ | |||
| } | |||
|
|
|||
| &.icon-bookmark-added { | |||
| background-image: url('chrome://browser/skin/bookmark.svg'); | |||
| background-image: url('chrome://browser/skin/bookmark.svg'); | |||
| } | |||
There was a problem hiding this comment.
Some of these close braces } need to be indented 1 more space to match the opening brace line.
There was a problem hiding this comment.
Changes updated in c882ccf
| &:hover, &:focus, &.active { | ||
| &:hover, | ||
| &:focus, | ||
| &.active { |
There was a problem hiding this comment.
I wonder if we should switch these types of selectors to something like &:-moz-any(.active, :focus, :hover) {. The generated css before/after:
- .top-sites-list .top-site-outer:hover .tile, .top-sites-list .top-site-outer:focus .tile, .top-sites-list .top-site-outer.active .tile {
+ .top-sites-list .top-site-outer:-moz-any(.active, :hover, :focus) .tile {
- .top-sites-list .top-site-outer:hover .edit-menu, .top-sites-list .top-site-outer:focus .edit-menu, .top-sites-list .top-site-outer.active .edit-menu {
+ .top-sites-list .top-site-outer:-moz-any(.active, :hover, :focus) .edit-menu {
- .top-sites-list .top-site-outer:hover .context-menu-button, .top-sites-list .top-site-outer:focus .context-menu-button, .top-sites-list .top-site-outer.active .context-menu-button {
+ .top-sites-list .top-site-outer:-moz-any(.active, :hover, :focus) .context-menu-button {There was a problem hiding this comment.
Increased brevity is always a plus - let me know if you'd like for the changes to be implemented and I'll get on it!
| fill: $fill-primary; | ||
| } | ||
|
|
||
| &[aria-expanded='true'] + .info-option { |
There was a problem hiding this comment.
With the refactoring above, this should be able to be nested in the above section &[aria-expanded='true'] { as + .info-option {
There was a problem hiding this comment.
Resolved with commit ff5b0da!
| @@ -59,7 +62,7 @@ | |||
| &::after { | |||
| border-bottom: 1px solid rgba(0, 0, 0, 0.05); | |||
| bottom: 0; | |||
| content: " "; | |||
| content: ' '; | |||
There was a problem hiding this comment.
Any idea if having content: '' vs content: ' ' makes any differences in the places we're touching here?
There was a problem hiding this comment.
That's a good question - I'm now wondering that myself.
I assume an empty set of single quotes would render no characters at all and for the latter, a single white space (ASCII value of 32).
Question that needs to be asked now is if we need that single white space and what purpose does it serve?
Edit: Food for thought - would content: none be more appropriate? (MDN docs)
| @@ -75,7 +75,7 @@ | |||
|
|
|||
| &.icon-trending { | |||
| background-image: url('#{$image-path}glyph-trending-16.svg'); | |||
| transform: translateY(2px); /* trending bolt is visually top heavy */ | |||
| transform: translateY(2px); // trending bolt is visually top heavy | |||
There was a problem hiding this comment.
I suppose to be explicit, the // quotes are removed from the generated file whereas /* */ comments remain. Just making sure that's desired. @k88hudson ?
| @@ -47,7 +47,7 @@ | |||
| fill: $fill-primary; | |||
| } | |||
|
|
|||
| &[aria-expanded='true'] + .info-option { | |||
| .info-option { | |||
There was a problem hiding this comment.
These two are not the same before/after. Note the +.
There was a problem hiding this comment.
Fixed with commit cb06173
|
On that note of the previous commit, how have you been testing these changes to make sure they don't regress look and functionality? |
|
@Mardak I had |
|
@seanprashad thanks for the work here. I rebased your commits in #3871 as the sass-lint dependency was out of date and there were some recent additions. I'll squash the changes into your first commit that you authored. |
Resolves #3409 - Sass linting was raising multiple issues (96 issues - 22 errors & 74 warnings).
To resolve this bug, I performed the following steps:
sass-lintglobally via npmsass-lint -c .sass-lint.yml 'system-addon/content-src/**/*.scss' -v -q(Note: the
-vargument specifies verbose output)npm run startmcin order to build copies of my changes tomozilla-centrallocallysass-lintdocumentation (/docs/rules) to identify solutions with respect to each errornpm run testmcwhich produced the following output: