Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed

Conversation

@abhishekbose87
Copy link

@abhishekbose87 abhishekbose87 commented Jun 18, 2016
edited by justin808
Loading

Let me know if this looks good.

Issues that I notice

  • react-bootstrap module does not support bootstrap. Possible alternative is to use react-bootstrap-4
  • navbar is not looking that good on mobile

This change is Reviewable

Copy link
Member

@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

Copy link
Member

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

Copy link
Author

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.

Copy link
Member

@abhishekbose87 Please try running the precompile locally.

Copy link
Member

@abhishekbose87 Any update? This PR sounds very useful!

Copy link
Member

jbhatab commented Jul 8, 2016

@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?


# Major version of Bootstrap: 3 or 4
bootstrapVersion: 3
bootstrapVersion: 4
Copy link
Member

@justin808 justin808 Jul 8, 2016

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.

Copy link
Member

@jbhatab The point of this tutorial is to show real world integrations of the shakacode open source including:

Copy link
Member

jbhatab commented Jul 8, 2016

wrong repo.... my bad!

Still curious about the compilation issue.

Copy link
Author

@justin808 Sorry I got busy and conveniently forgot about it 😊

Let me provide an update by end of this weekend.

Copy link
Author

@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

Copy link
Member

justin808 commented Jul 17, 2016
edited
Loading

@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

Copy link
Author

Copy link
Member

@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

Copy link
Member

@abhishekbose87 Let's get this merged to master! I need you to rebase on top of master, and then I'll merge it!

Copy link
Author

@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.

Copy link
Member

Copy link
Member

@abhishekbose87 Any opinion on if we should wait until react-bootstrap officially supports bootstrap v4?

Copy link
Member

@AlexKVal, what's your opinion of this? This project shows off react-bootstrap...

Copy link
Contributor

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.

last-commit

npmjs

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

🍒

Copy link
Member

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.

Copy link
Member

Would be great to get this updated! See #238.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /