Skip to content
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

JavaScript style guide draft #61

Merged
merged 11 commits into from
Jan 28, 2021
Merged

JavaScript style guide draft #61

merged 11 commits into from
Jan 28, 2021

Conversation

aalin
Copy link
Contributor

@aalin aalin commented Dec 22, 2020

No description provided.

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Love that we are getting a document home for this.


Since then, npm has improved a lot and nowadays there isn't really any reason to use yarn over npm.

[Why Are You Still Using Yarn in 2018?](https://iamturns.com/yarn-vs-npm-2018/)
Copy link
Contributor

Choose a reason for hiding this comment

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

One feature in yarn that I use is resolutions, and I don't know of a way to control that in npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never used it myself!

I guess if we have a situation where we need to use that feature we could switch to yarn for that project. After all, this is just a guideline. But I feel like it's better to use npm which comes with most nodejs-installs instead of using a third-party package manager that does 99% of the same things.

Copy link
Member

Choose a reason for hiding this comment

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

styleguide/javascript/README.md Outdated Show resolved Hide resolved
styleguide/javascript/README.md Show resolved Hide resolved
styleguide/javascript/README.md Outdated Show resolved Hide resolved
styleguide/javascript/README.md Outdated Show resolved Hide resolved
styleguide/javascript/README.md Show resolved Hide resolved
styleguide/javascript/README.md Outdated Show resolved Hide resolved
aalin and others added 4 commits December 22, 2020 12:50
Co-authored-by: Olle Jonsson <olle.jonsson@auctionet.com>
Co-authored-by: Olle Jonsson <olle.jonsson@auctionet.com>
@henrik
Copy link
Member

henrik commented Jan 11, 2021

We should link to this file from styleguide/README.md under the JavaScript section (and remove its "Prefer CoffeeScript" stuff).

Copy link
Member

@henrik henrik left a comment

Choose a reason for hiding this comment

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

As the starting-off point for a living document this seems fine to me – it's quite far from where we are today (with a lot of CoffeeScript and jQuery) but I like that it's aspirational, and we can always tweak it as we go.

It would be good if we could edit/remove those old CoffeeScripty parts of the styleguide though so we don't have two mutually exclusive recommendations living side by side :)

styleguide/javascript/README.md Outdated Show resolved Hide resolved
styleguide/javascript/README.md Outdated Show resolved Hide resolved

#### Dependencies

In the npm repository, you can find packages for almost anything.
Copy link
Member

Choose a reason for hiding this comment

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

This phrasing of this section reads a bit weird to me – I feel like a more prescriptive style makes sense in a styleguide – more like "Avoid adding npm dependencies for simple things [reasons and examples given here]".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point :)

styleguide/javascript/README.md Show resolved Hide resolved
styleguide/javascript/README.md Outdated Show resolved Hide resolved

## React

The other guide is written for CoffeeScript so here's one for JavaScript.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the other guide?

styleguide/javascript/README.md Outdated Show resolved Hide resolved
Copy link
Member

@tskogberg tskogberg left a comment

Choose a reason for hiding this comment

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

Nice!

@aalin
Copy link
Contributor Author

aalin commented Jan 18, 2021

We should link to this file from styleguide/README.md under the JavaScript section (and remove its "Prefer CoffeeScript" stuff).

@henrik f162fc2

@tskogberg tskogberg merged commit 335b0f4 into master Jan 28, 2021
@tskogberg tskogberg deleted the javascript branch January 28, 2021 14:56
aalin added a commit that referenced this pull request Jan 29, 2021
This reverts commit 335b0f4, reversing
changes made to a2bfb55.
olleolleolle added a commit that referenced this pull request Mar 12, 2021
This reverts commit bf882c5.

We are interested in the JS styleguide. Re-opening the PR.
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.

5 participants