-
Notifications
You must be signed in to change notification settings - Fork 378
Upgrade bootstrap and add example of font-awesome #294
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
Conversation
@jbhatab Please check this out.
Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.
Comments from Reviewable
A couple questions below. Once you have something workable, can you push to your own Heroku instance?
Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.
client/package.json, line 79 [r1] (raw file):
"react": "^0.14.8", "react-addons-pure-render-mixin": "^0.14.8", "react-bootstrap": "^0.28.5",
what do we do about react-bootstrap?
client/webpack.client.base.config.js, line 49 [r1] (raw file):
new webpack.ProvidePlugin({ 'window.Tether': 'tether'
what's this one do?
Comments from Reviewable
I am unsuccesful in pushing the app to Heroku. Can you help me out here ?
I am getting following error message ..
remote: Running: rake assets:precompile
remote: cd client && npm run build:production
remote: sh: 1: npm: not found
remote: rake aborted!
remote: Command failed with status (127): [cd client && npm run build:production...]
remote: /tmp/build_dbacc15ff61bc51733b3d85f884b771d/vendor/bundle/ruby/2.3.0/gems/react_on_rails-6.0.2/lib/tasks/assets.rake:91:in `block (3 levels) in <top (required)>'
remote: Tasks: TOP => assets:precompile => react_on_rails:assets:compile_environment => react_on_rails:assets:webpack
I am using tether because of following documentation.
https://github.com/shakacode/bootstrap-loader/blob/40788916f0eb822baa09a5d08beee7e4d9608ebb/README.md#tether
And we can replace react-bootstrap with react-bootstrap-4.
@abhishekbose87 Please try running the precompile locally.
@abhishekbose87 Any update? This PR sounds very useful!
@justin808 we removed bootstrap entirely from the gem. We also removed all css based things like font decisions so people can make those decisions on their own. Not sure about merging this one.
@abhishekbose87 did that compilation error start when you added bootstrap with tether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbhatab This is the main change. Modernizing to BS4.
@jbhatab The point of this tutorial is to show real world integrations of the shakacode open source including:
wrong repo.... my bad!
Still curious about the compilation issue.
@justin808 Sorry I got busy and conveniently forgot about it 😊
Let me provide an update by end of this weekend.
@justin808 I tried precompiling the assets and then pushing it to heroku. But it gives the same error.
remote: cd client && npm run build:production
remote: sh: 1: npm: not found
@abhishekbose87 did you see: https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/heroku-deployment.md
heroku buildpacks:set heroku/ruby
heroku buildpacks:add --index 1 heroku/nodejs
@justin808 Thanks. Got it up and running - https://shrouded-garden-10771.herokuapp.com/
@abhishekbose87 Please rebase on top of master. Be sure that you are only moving library versions upwards.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.
Comments from Reviewable
@abhishekbose87 Let's get this merged to master! I need you to rebase on top of master, and then I'll merge it!
858bf3c to
d149643
Compare
@justin808 I have rebased master and fixed the conflicts. The new version is deployed to https://shrouded-garden-10771.herokuapp.com/
Let me know if you see any issues.
@abhishekbose87 You have linter errors
@abhishekbose87 Any opinion on if we should wait until react-bootstrap officially supports bootstrap v4?
@AlexKVal, what's your opinion of this? This project shows off react-bootstrap...
The React-Bootstrap team's official stance on supporting Bootstrap@4 read here:
- we will support bootstrap 4
- we will not start that support until bootstrap 4 is stable enough that we can reasonably develop against it without it constantly changing.
The reason for it is a lot of additional maintenance burden.
A good example how it usually goes. 😈
Someone (it is usually a very kind developer with very good intentions 🍒 ) does (a lot of) work
to implement something like that.
(Adding as a dependency a project that is still in beta stage.)
react-bootstrap/react-bootstrap#1187 (comment)
And a couple of weeks later he/she realizes that although there are A LOT of people who would like to USE the upgraded implementation,
at the same time there are (usually) no maintainers who is ready to dedicate their time to... maintain it.
And then happens this:
github: Latest commit d530918 on May 13
npmjs: react-bootstrap-4 is published 3 months ago.
The author drops his/her undertaking.
If someone from the react-webpack-rails-tutorial's team would merge this upgrade,
then he/she becomes obliged to maintain it all the way down 😈
If nobody from the core team of the react-webpack-rails-tutorial project is ready
to dedicate their time to maintain Bootstrap@4-beta dependency in the project now,
then... it is open-source at the end of the day.
Anyone anytime can create fork and maintain Bootstrap@4 version but everyone
knows that this project is just a tutorial and it is not worth of efforts.
Though exactly this PR shows very good intentions from @abhishekbose87
🍒
Let's keep this on in the PR bucket until bootstrap 4 goes GA! This PR will serve as a reference for those that wish to try out BS4 sooner.
Would be great to get this updated! See #238.
Uh oh!
There was an error while loading. Please reload this page.
Let me know if this looks good.
Issues that I notice
react-bootstrapmodule does not support bootstrap. Possible alternative is to usereact-bootstrap-4This change is Reviewable