Skip to content

Eslint and stylelint all .vue files#1482

Merged
ang-zeyu merged 5 commits into
MarkBind:masterfrom
jonahtanjz:enable-linting-vue
Feb 23, 2021
Merged

Eslint and stylelint all .vue files#1482
ang-zeyu merged 5 commits into
MarkBind:masterfrom
jonahtanjz:enable-linting-vue

Conversation

@jonahtanjz

@jonahtanjz jonahtanjz commented Feb 18, 2021

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
A follow-up PR to #1462. Enable eslint and stylelint for all .vue files in vue-components.

Anything you'd like to highlight / discuss:

  1. There are a few console.logs used which eslint will throw a warning. Should I disable the checks for these console.logs or remove them?
  2. Use of v-html in some .vue files is also throwing warnings during the check. Should this be change from <span v-html="html"></span> to <span>{{ html }}</span> instead to follow eslint rules?

Testing instructions:
npm run test

Proposed commit message: (wrap lines at 72 characters)
Eslint and stylelint all .vue files

Currently, most .vue files are ignored during the linting checks.

Since these files are frequently modified, we should validate
them in our ci tests to ensure they follow eslint rules. Furthermore,
.vue files can contain raw css hence appropriate to also stylelint them.

Let's eslint and stylelint all .vue files so that they can be validated
as well.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@jonahtanjz jonahtanjz changed the title Enable linting vue Eslint and stylelint all .vue files Feb 18, 2021
beforeDestroy() {
if (this._tabset.active === this.index) { this._tabset.active = 0; }
if (this._ingroup) {
var index = parent.tabs.indexOf(this);

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.

👀 should this have been referencing $parent?

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.

Yup seems like it should have been $parent as the function parent.tabs.indexOf does not exist.

@ang-zeyu ang-zeyu left a comment

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.

Lgtm!

There are a few console.logs used which eslint will throw a warning. Should I disable the checks for these console.logs or remove them?
Use of v-html in some .vue files is also throwing warnings during the check. Should this be change from to {{ html }} instead to follow eslint rules?

Thanks for the catch, I think we can resolve the second issue separately since it requires a substantial functional change.

For the console.logs, might be better to remove them as we shouldn't be relying on it to inform users of anything. Where appropriate, could replace them with comments for anyone taking a look at these files in the future.

@jonahtanjz

Copy link
Copy Markdown
Contributor Author

@ang-zeyu Updated the PR to remove the consoles or replaced them with comments.

@ang-zeyu ang-zeyu added this to the v3.0 milestone Feb 23, 2021
@ang-zeyu ang-zeyu merged commit e1e090f into MarkBind:master Feb 23, 2021
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.

2 participants