Fix #3409 - Sass Linting Errors #3751

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
2 participants
@SeanPrashad
Contributor

SeanPrashad commented Oct 22, 2017

Resolves #3409 - Sass linting was raising multiple issues (96 issues - 22 errors & 74 warnings).

To resolve this bug, I performed the following steps:

  • Installed sass-lint globally via npm
    • Utilized the built-in CLI debugging to diagnose and fix each issue via the following command:
      sass-lint -c .sass-lint.yml 'system-addon/content-src/**/*.scss' -v -q
      (Note: the -v argument specifies verbose output)
  • Ran npm run startmc in order to build copies of my changes to mozilla-central locally
  • Referred to the sass-lint documentation (/docs/rules) to identify solutions with respect to each error
  • After all errors & warnings were resolved, I ran npm run testmc which produced the following output:
    runningunittests
@@ -18,39 +18,39 @@
}
&.icon-bookmark-added {
- background-image: url('chrome://browser/skin/bookmark.svg');
+ background-image: url('chrome://browser/skin/bookmark.svg');
}

This comment has been minimized.

@Mardak

Mardak Oct 22, 2017

Member

Some of these close braces } need to be indented 1 more space to match the opening brace line.

@Mardak

Mardak Oct 22, 2017

Member

Some of these close braces } need to be indented 1 more space to match the opening brace line.

This comment has been minimized.

@SeanPrashad

SeanPrashad Oct 22, 2017

Contributor

Changes updated in c882ccf

@SeanPrashad

SeanPrashad Oct 22, 2017

Contributor

Changes updated in c882ccf

- &:hover, &:focus, &.active {
+ &:hover,
+ &:focus,
+ &.active {

This comment has been minimized.

@Mardak

Mardak Oct 23, 2017

Member

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 {
@Mardak

Mardak Oct 23, 2017

Member

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 {

This comment has been minimized.

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

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!

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

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 {

This comment has been minimized.

@Mardak

Mardak Oct 23, 2017

Member

With the refactoring above, this should be able to be nested in the above section &[aria-expanded='true'] { as + .info-option {

@Mardak

Mardak Oct 23, 2017

Member

With the refactoring above, this should be able to be nested in the above section &[aria-expanded='true'] { as + .info-option {

This comment has been minimized.

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

Resolved with commit ff5b0da!

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

Resolved with commit ff5b0da!

@@ -59,7 +62,7 @@
&::after {
border-bottom: 1px solid rgba(0, 0, 0, 0.05);
bottom: 0;
- content: " ";
+ content: ' ';

This comment has been minimized.

@Mardak

Mardak Oct 23, 2017

Member

Any idea if having content: '' vs content: ' ' makes any differences in the places we're touching here?

@Mardak

Mardak Oct 23, 2017

Member

Any idea if having content: '' vs content: ' ' makes any differences in the places we're touching here?

This comment has been minimized.

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

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)

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

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

This comment has been minimized.

@Mardak

Mardak Oct 23, 2017

Member

I suppose to be explicit, the // quotes are removed from the generated file whereas /* */ comments remain. Just making sure that's desired. @k88hudson ?

@Mardak

Mardak Oct 23, 2017

Member

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 {

This comment has been minimized.

@Mardak

Mardak Oct 23, 2017

Member

These two are not the same before/after. Note the +.

@Mardak

Mardak Oct 23, 2017

Member

These two are not the same before/after. Note the +.

This comment has been minimized.

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

Fixed with commit cb06173

@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

Fixed with commit cb06173

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 23, 2017

Member

On that note of the previous commit, how have you been testing these changes to make sure they don't regress look and functionality?

Member

Mardak commented Oct 23, 2017

On that note of the previous commit, how have you been testing these changes to make sure they don't regress look and functionality?

@SeanPrashad

This comment has been minimized.

Show comment
Hide comment
@SeanPrashad

SeanPrashad Oct 23, 2017

Contributor

@Mardak I had npm run startmc running in the background during earlier commits and built activity stream after making changes via npm run buildmc. I then built and ran my local version of Firefox and opened a new tab to see if anything had broken. I didn't for the previous commit but will make sure to test changes for each commit in the future.

Contributor

SeanPrashad commented Oct 23, 2017

@Mardak I had npm run startmc running in the background during earlier commits and built activity stream after making changes via npm run buildmc. I then built and ran my local version of Firefox and opened a new tab to see if anything had broken. I didn't for the previous commit but will make sure to test changes for each commit in the future.

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Nov 21, 2017

Member

@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.

Member

Mardak commented Nov 21, 2017

@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.

@Mardak Mardak closed this Nov 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment