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

Upgrading versions and use the sample-template-rails #65

Merged
merged 13 commits into from
Jun 30, 2020
Merged

Conversation

rojasTob
Copy link
Contributor

@rojasTob rojasTob commented Jun 22, 2020

Resolves #64

This project was updated using the sample-template-rails. Additionally, I've added the Makefile and docker-compose.

Copy link

@po5i po5i left a comment

Choose a reason for hiding this comment

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

According to GitHub's gitignore samples The following directories shouldn't be in the git tree:

/public/packs
/public/packs-test
/public/assets

You may want to review that .gitignore sample 😉

@rojasTob
Copy link
Contributor Author

rojasTob commented Jun 23, 2020

/public/packs
/public/packs-test
/public/assets

That is correct, thanks for pointing this out :) I will review it.

Copy link

@mmena1 mmena1 left a comment

Choose a reason for hiding this comment

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

Please remove the webpack compiled assets from the /public folder. Also be aware that here you are only serving the bootstrap assets with webpack. You may need to move the assets from app/assets to app/javascript and import them in app/javascript/packs/application.js. See this discussion for reference.

@rojasTob rojasTob requested a review from mmena1 June 29, 2020 16:33
Copy link

@mmena1 mmena1 left a comment

Choose a reason for hiding this comment

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

This is looking good! I only left a couple of minor comments. Also, it would be nice if bootstrap can be served from node instead from ruby gems, but it's ok as it is.

Gemfile Outdated
Comment on lines 78 to 80
gem 'vcr'
gem 'webdrivers'
gem 'webmock'
Copy link

Choose a reason for hiding this comment

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

This gems are unused. In fact, since there are no e2e tests, I believe the whole :test group is not used. Can you confirm that?

Gemfile Outdated
gem 'overcommit', '~> 0.52.1', require: false
gem 'rubocop', '~> 0.80.0', require: false
gem 'rubocop-rails', '~> 2.4', require: false
gem 'rubocop-rspec', require: false
Copy link

Choose a reason for hiding this comment

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

Since we are not using rspec, this shouldn't be needed.

@rojasTob rojasTob merged commit 0440a96 into next Jun 30, 2020
@maylonpedroso maylonpedroso deleted the upgrade-app branch October 26, 2020 21:10
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.

3 participants