Skip to content

Fix all stylelint errors - #23920

Merged
abutterworth merged 3 commits into
masterfrom
abutterworth/fix-style-lint-errors
May 6, 2020
Merged

Fix all stylelint errors#23920
abutterworth merged 3 commits into
masterfrom
abutterworth/fix-style-lint-errors

Conversation

@abutterworth

@abutterworth abutterworth commented May 6, 2020

Copy link
Copy Markdown
Contributor

For any errors that fixing would require changing the output of the css, this PR disables stylelint for that line instead of modifying. Example:

body:before { /* stylelint-disable-line */
  ...

Rather than fixing the selector

body::before

Basically, if it was broken, leave it broken for now. This approach should prevent any unexpected changes to the css output.

FYI @AmalBas

For any errors that fixing would require changing the output of the css disable stylelint for that line instead of modifying.
Make stylelint quality check pass when there are no errors
Comment thread pavelib/quality.py
This will return the number in the last line of a file.
It is returning only the value (as a floating number).
"""
last_line = _get_report_contents(filename, file_type, last_line_only=True).strip()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line throw an error when there are no lint errors

Comment thread pavelib/quality.py
report_contents = _get_report_contents(filename, file_type, last_line_only=True)

if report_contents is None:
return 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning 0 as the count if there is no content. Curious if this generates concern from anyone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmbowman fyi.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this generates concern

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it turns out, if the stylelint command is not found, then the report ends up being empty.

Comment thread cms/static/sass/_base.scss Outdated

.nav-account-user {
.title {
max-width: ($baseline*10.5);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd that this just got deleted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted it because it's redeclared on 137

@abutterworth

abutterworth commented May 6, 2020 via email

Copy link
Copy Markdown
Contributor Author

Comment thread cms/static/sass/contexts/_ie.scss Outdated
Comment thread cms/static/sass/_base.scss Outdated
Comment thread cms/static/sass/views/_index.scss Outdated
Comment thread lms/static/sass/course/_textbook.scss Outdated
// Breadcrumbs

-----------------*/
// -----------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this also changing comment style?

Not a huge deal, but just to note, this now means the empty lines above are now outside of the comment(s), instead of inside. 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter doesn't allow comments like this one it seems.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

// ====================

// CASE: less than or equal to IE7
.lte7 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w00t!

// Breadcrumbs

-----------------*/
// -----------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

@abutterworth
abutterworth merged commit 42cc0d0 into master May 6, 2020
@abutterworth
abutterworth deleted the abutterworth/fix-style-lint-errors branch May 6, 2020 20:07
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants