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 bootstrap and popper.js dependencie, yarn.lock update #53

Closed
wants to merge 4 commits into from
Closed

Add bootstrap and popper.js dependencie, yarn.lock update #53

wants to merge 4 commits into from

Conversation

skoyah
Copy link
Contributor

@skoyah skoyah commented Nov 17, 2018

This PR fixes #52 .

@lucasmichot
Copy link
Contributor

@skoyah - should you not also commit the updated version of yarn.lock then?

@skoyah
Copy link
Contributor Author

skoyah commented Nov 17, 2018

@lucasmichot I'm kinda new when it comes to contributing, so I wasn't sure if the lock files should be committed to packages. Should I commit them?

@lucasmichot
Copy link
Contributor

No worries @skoyah 👍 - Yes I would also commit the updated version of yarn.lock
You can this extra commit to this PR

@skoyah
Copy link
Contributor Author

skoyah commented Nov 17, 2018

Ok @lucasmichot! Thanks for the info! Will do it asap!

@skoyah
Copy link
Contributor Author

skoyah commented Nov 17, 2018

So @lucasmichot one more thing, how can I add the yarn.lock file to this PR? or should I delete this PR and create a new one?

It seems the yarn.lock file was not modified. But I do have a package-lock.json file untracked.

I've made some research and some people recommend to commit both files so people can use npm or yarn on their projects. But some other people say that when contributing to packages you should never commit those files.

So maybe @themsaid could help us on this subject.

@skoyah skoyah changed the title Add bootstrap and popper.js dependencies Add bootstrap and popper.js dependencie, yarn.lock update Nov 17, 2018
@themsaid
Copy link
Owner

But we're not using Bootstrap anymore. We use TailwindCSS now.

@themsaid themsaid closed this Nov 17, 2018
@skoyah
Copy link
Contributor Author

skoyah commented Nov 17, 2018

So @themsaid, that begs the question of maybe removing the unnecessary call to bootstrap an popper.js on the app.js file.

@skoyah skoyah mentioned this pull request Nov 18, 2018
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.

Error when compiling assets
3 participants