-
-
Notifications
You must be signed in to change notification settings - Fork 180
Linting and code formatting #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linting and code formatting #321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, there are a lot of changes in this PR, thank you for your work ! :)
I have added some comments that need to be addressed inline.
package.json
Outdated
| "lint": "vue-cli-service lint", | ||
| "i18n:report": "vue-cli-service i18n:report -v --src src/**/*.vue --locales src/locales/**.json -o output.json", | ||
| "lint:js": "vue-cli-service lint", | ||
| "stylelint": "stylelint \"**/*.{vue,css}\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make stylelint only lint the content of 'src' folder? Otherwise it also tries to lint 'coverage', 'dist', or any other folders we might add later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's ignoreFiles: ['docs/css/*'] property in stylelint.config.js which will prevent stylelint to lint this folder. So in future we can add other folder here. But still I'll make stylelint only lint the content of 'src' folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also does this mean eslint is also linting dist/js/* or vue-cli-service will handle it and will lint-staged lint 'dist' while committing or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I @rohan-goel we don't want the /dist folder linted, only the src/ files we write. I think @obulat's original approach to specify src/ is still the best approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zackkrida I have add "lint": "vue-cli-service lint && npx stylelint ./src/**/*.{vue,css} --fix" command in package.json. This will only lint src folder.
|
@rohan-goel Update your brach to resolve the conflicts and also take a |
|
@Cronus1007 I resolved merge conflicts but now I'm getting another test failed. Can you tell me what went wrong?? |
|
@rohan-goel Have a look at these two ss . One is from the code of cc-chooser repo and one from yours. You mistakenly made few changes in the |
|
Thanks for your help @Cronus1007 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohan-goel Plzzz update your branch to resolve the merge conflicts.
# Conflicts: # src/components/AttributionDetailsStep.vue # src/components/LicenseDetailsCard.vue # src/components/LicenseText.vue # src/store/index.js # src/utils/license-utilities.js # tests/unit/specs/components/AttributionDetailsStep.spec.js # tests/unit/specs/components/LicenseText.spec.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, @rohan-goel ! The code looks much cleaner now :)
|
Thanks, @obulat !! |


Fixes
Fixes #317 by @obulat
Description
Checklist
Update index.md).mainormaster).visible errors.
Developer Certificate of Origin
Developer Certificate of Origin