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

Update Bootstrap version #151

Merged
merged 5 commits into from
Nov 20, 2017
Merged

Update Bootstrap version #151

merged 5 commits into from
Nov 20, 2017

Conversation

chocoelho
Copy link
Contributor

@chocoelho chocoelho commented Nov 17, 2017

Description

This PR updates supported Bootstrap 4 version from alpha to beta. normalize was removed from beta version, so we don't need that in .bootstraprc anymore.

Motivation and Context

Resolves #125. We upgrade from alpha to beta version of Bootstrap 4.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires documentation updates.
  • I have updated the documentation accordingly.
  • My change requires dependencies updates.
  • I have updated the dependencies accordingly.

Copy link
Contributor

@aericson aericson left a comment

Choose a reason for hiding this comment

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

Hey @chocoelho

Did you check the changes in shakacode/bootstrap-loader#316 ?

I only did a quick look but a few changes that might require researching into why they made them:

  • Removed responsive-embed: true from .bootstraprc-4-default
  • Removed useFlexbox: true and all mentions/documentation of it. We use useFlexbox: false. Does that still work? I think this won't work because of this change. If so, how can we disable it and do we still have a valid reason to disable it? Maybe @laisvarejao can help.
  • They changed the order of sass-loader and postcss-loader in their example. They might have had a good reason to do it.

If you have taken those into consideration and they don't require any further changes disconsider it.

@chocoelho
Copy link
Contributor Author

chocoelho commented Nov 17, 2017

Thanks, @aericson. I also noticed that I had forgotten to include Popper.js configuration in webpack.

As to what you said:

Removed responsive-embed: true from .bootstraprc-4-default

  • I just removed that, even though it was set to false, but that's not working anymore

Removed useFlexbox: true and all mentions/documentation of it. We use useFlexbox: false. Does that still work? I think this won't work because of this change. If so, how can we disable it and do we still have a valid reason to disable it? Maybe @laisvarejao can help.

  • They removed useFlexbox option since BS4 uses flexboxes by default. I don't know if that's something we can change right now.

They changed the order of sass-loader and postcss-loader in their example. They might have had a good reason to do it.

  • Basically, because order matters. Thanks for noticing that, I fixed it.

@laisvarejao
Copy link
Contributor

@aericson where did you see that the order of the sass-loader and postcss-loader changed?
I don't see this in the example:
https://github.com/shakacode/bootstrap-loader/blob/master/.bootstraprc-4-default

@aericson @chocoelho yes, Flexbox is now the only option! This has changed since Alpha 6:

Flexbox is now on by default, with no fallback! This means we've dropped IE9 support, but with significant savings to our code base, simpler components, and improved customization thanks to the power of flexbox. (twbs/bootstrap#20939)

  • I've also noticed they are alphabetically ordering the styles components in bootstrap.rc. I think this is a good standard to follow.

@chocoelho
Copy link
Contributor Author

@laisvarejao about the changed order: shakacode/bootstrap-loader#316 (comment)

@chocoelho
Copy link
Contributor Author

Regarding the alphabetic order, that can be done as well and I agree with it.

@chocoelho chocoelho merged commit 858d855 into master Nov 20, 2017
@chocoelho chocoelho deleted the update-bootstrap branch November 20, 2017 20:39
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.

Update to Bootstrap 4 beta
3 participants