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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

UMD + Linting #15

Merged
merged 3 commits into from Apr 6, 2017

Conversation

Projects
None yet
3 participants
@stephane-tessier
Contributor

stephane-tessier commented Apr 6, 2017

Hi,

Thanks for the great job! 馃憤

I suggest turning this module into a Universal Module, so that it can be included more easily into some projects.
Besides, some linting let me see that you declare variables that you don't use ;)

@nico3333fr

This comment has been minimized.

Show comment
Hide comment
@nico3333fr

nico3333fr Apr 6, 2017

Owner

Hi,

thanks :)

yes, this script needs a lot of cleanup, you've made a good start here :) (it is one of the oldest I've written)

I hope to perform a full and clean rewrite soon on it, as @ryuran did for accordion script https://github.com/nico3333fr/jquery-accessible-accordion-aria

Quick question and advice needed: I've noticed you added .eslintrc.json and @ryuran used .jshintrc .

ShouId I put both on the repo ?

Anyway, thanks a lot for your help 馃憤

Owner

nico3333fr commented Apr 6, 2017

Hi,

thanks :)

yes, this script needs a lot of cleanup, you've made a good start here :) (it is one of the oldest I've written)

I hope to perform a full and clean rewrite soon on it, as @ryuran did for accordion script https://github.com/nico3333fr/jquery-accessible-accordion-aria

Quick question and advice needed: I've noticed you added .eslintrc.json and @ryuran used .jshintrc .

ShouId I put both on the repo ?

Anyway, thanks a lot for your help 馃憤

@nico3333fr nico3333fr self-assigned this Apr 6, 2017

@ryuran

This comment has been minimized.

Show comment
Hide comment
@ryuran

ryuran Apr 6, 2017

@nico3333fr jshint will be disappear for eslint.
https://www.slant.co/versus/8627/8628/~jshint_vs_eslint

Furthermore jshint do not control coding style and need jscs to complete this missing.

But jscs has merged with eslint. http://jscs.info/
So eslint covers all the scope for linting and code styling control.

ryuran commented Apr 6, 2017

@nico3333fr jshint will be disappear for eslint.
https://www.slant.co/versus/8627/8628/~jshint_vs_eslint

Furthermore jshint do not control coding style and need jscs to complete this missing.

But jscs has merged with eslint. http://jscs.info/
So eslint covers all the scope for linting and code styling control.

[*.{js,scss,css,html,hbs,twig,json}]
indent_style = space
indent_size = 4

This comment has been minimized.

@ryuran

ryuran Apr 6, 2017

Missing new line

@ryuran

ryuran Apr 6, 2017

Missing new line

This comment has been minimized.

@stephane-tessier

stephane-tessier Apr 6, 2017

Contributor

fixed, thanks!

@stephane-tessier

stephane-tessier Apr 6, 2017

Contributor

fixed, thanks!

@nico3333fr nico3333fr merged commit 0006e4b into nico3333fr:master Apr 6, 2017

@nico3333fr

This comment has been minimized.

Show comment
Hide comment
@nico3333fr

nico3333fr Apr 6, 2017

Owner

@ryuran so it is better to use settings provided in this example ? I guess I will have to update them in accordion.

Both @ryuran and @stephane-tessier : Thanks a lot for your help 馃憤

Owner

nico3333fr commented Apr 6, 2017

@ryuran so it is better to use settings provided in this example ? I guess I will have to update them in accordion.

Both @ryuran and @stephane-tessier : Thanks a lot for your help 馃憤

@stephane-tessier

This comment has been minimized.

Show comment
Hide comment
@stephane-tessier

stephane-tessier Apr 7, 2017

Contributor

馃槃

Contributor

stephane-tessier commented Apr 7, 2017

馃槃

@ryuran

This comment has been minimized.

Show comment
Hide comment
@ryuran

ryuran Apr 7, 2017

@nico3333fr This one is set to 4 space indent size.
For accordion is set to 2.
You need to make some choice about the coding style.
It can be a good idea to put the eslint config on a specific package http://eslint.org/docs/user-guide/configuring#using-a-shareable-configuration-package and document your coding style here in this repo.

Each project of your collection will refer to this external settings and will respect same rules.

ryuran commented Apr 7, 2017

@nico3333fr This one is set to 4 space indent size.
For accordion is set to 2.
You need to make some choice about the coding style.
It can be a good idea to put the eslint config on a specific package http://eslint.org/docs/user-guide/configuring#using-a-shareable-configuration-package and document your coding style here in this repo.

Each project of your collection will refer to this external settings and will respect same rules.

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