Skip to content

Enable linting for dropdown.vue#1462

Merged
ang-zeyu merged 4 commits into
MarkBind:masterfrom
jonahtanjz:enable-linting-for-dropdown
Jan 30, 2021
Merged

Enable linting for dropdown.vue#1462
ang-zeyu merged 4 commits into
MarkBind:masterfrom
jonahtanjz:enable-linting-for-dropdown

Conversation

@jonahtanjz

@jonahtanjz jonahtanjz commented Jan 28, 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:
This is a follow-up PR to #1455. Linting for dropdown.vue is enabled and code is refactored to be consistent with the lint rules.

Anything you'd like to highlight / discuss:
Previously, there is a class which is set to null in the props. Not sure what is the function of it as it seems to work fine after removal. If it is needed, will proceed to add it back.

Testing instructions:
npm run test

Proposed commit message: (wrap lines at 72 characters)
Enable linting for dropdown.vue

Currently, stylelint is only enabled for css files. Vue components may
also contain <style> tags with valid css but these are not linted.

Let's enable stylelint css for vue files to provide linting to them 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

Copy link
Copy Markdown
Contributor Author

@ang-zeyu This PR is ready for review. Would appreciate if you can look through and give some feedbacks 😃

padding: 0;
border: 0;
}
.secret {

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.

looks good!

just one nit: let's enable and fix stylelint separately (for all .vue files that are es-linted)

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.

Sure 👍
Should I do it in this PR or open a separate one?

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.

let's do it separately, iirc none of the .vue files are validated in our ci tests - might be quite a few changes

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.

Alright. I've enabled stylelint for all .vue files but added all of them to .stylelintignore except for dropdown.vue for now. Will do it separately for all the other files in the next PR :)

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

Thanks! @jonahtanjz

Comment thread .stylelintignore

# --- packages/vue-components .vue files ---

packages/vue-components/src/Navbar.vue

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.

meant to shift all of stylelint to another PR, so you wouldn't have to write this huge ignore list here 😅

but am fine with this as well.

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.

Enable linting for dropdown.vue

let's update the message body to highlight vue css lint enabling as well

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.

Thanks @ang-zeyu. Updated the message body.

@ang-zeyu ang-zeyu added this to the v3.0 milestone Jan 30, 2021
@ang-zeyu ang-zeyu merged commit 606a155 into MarkBind:master Jan 30, 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