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

Add note for using yarn if using webpacker #582

Merged
merged 2 commits into from Sep 17, 2020
Merged

Add note for using yarn if using webpacker #582

merged 2 commits into from Sep 17, 2020

Conversation

mlodato517
Copy link

This PR
Adds a note to the README to yarn add i18n-js if using webpacker

Why?
I'm not actually sure if this is the right way to handle things but I followed the README for "without asset pipeline" and then got stuck because there was no I18n on the client. I was pointed to this gist and after adding the i18n-js package to the client, "everything worked".

If this isn't the right advice, I'd be happy to help update it to whatever it needs to be and/or point users in the right direction if this is already documented elsewhere!

@coveralls
Copy link

coveralls commented Sep 15, 2020

Pull Request Test Coverage Report for Build 682

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 68.519%

Totals Coverage Status
Change from base Build 680: 0.0%
Covered Lines: 296
Relevant Lines: 432

💛 - Coveralls

@TravisBuddy
Copy link

TravisBuddy commented Sep 15, 2020

Hey @mlodato517,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d0ec44b0-f818-11ea-8f9f-a900ee46d44b

@PikachuEXE
Copy link

Thanks for the PR
I think it better be moved to after Add the gem to your Gemfile. and before Rails app with Asset Pipeline

Since it's "optional step for installation"

Copy link

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

We should cover all common package managers not just yarn (even though I use yarn)

README.md Outdated
If you're using `webpacker`, you may need to add the dependencies to your client with:

```
yarn add i18n-js

Choose a reason for hiding this comment

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

# Depends on your package manager
npm install i18n-js
# or
yarn add i18n-js

Copy link
Author

Choose a reason for hiding this comment

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

Okay! I chose yarn since it's what webpacker uses and you have to go through some hoops to use npm with webpacker but this sounds good!

Copy link
Author

Choose a reason for hiding this comment

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

@PikachuEXE PikachuEXE merged commit 7535a9f into fnando:master Sep 17, 2020
@PikachuEXE
Copy link

Thanks for your contribution
Welcome to make more if you see fit

@mlodato517 mlodato517 deleted the ml-webpacker-docs branch September 17, 2020 12:48
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.

None yet

4 participants