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

Added support Bootstrap 4 Beta #316

Merged
merged 5 commits into from
Aug 18, 2017
Merged

Added support Bootstrap 4 Beta #316

merged 5 commits into from
Aug 18, 2017

Conversation

IvanKalinin
Copy link
Contributor

@IvanKalinin IvanKalinin commented Aug 13, 2017

This change is Reviewable

@justin808
Copy link
Member

Is there a good reason to even support the alpha? We'll eventually move to only support the released version.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@IvanKalinin
Copy link
Contributor Author

IvanKalinin commented Aug 13, 2017

I think so.
Beta has a lot of changed. A lot of variables were renamed, so I think a lot of developers still use alpha 6. It could take a time to update their own project from alpha to beta. And we cannot break someones projects with updates

In my thoughts we can drop alpha in the next beta release

If you think I have to drop alpha - will do

Fixed source object name for user config
@justin808
Copy link
Member

@IvanKalinin Great work!

  1. Let's remove all the beta parts. Alpha users should not upgrade.
  2. Please update the CHANGELOG.md per prior entries for v2.2.0 and make any necessary changes in the README.md
  3. Please confirm that you've tried all the examples in the examples directory and you've made the appropriate updates there so they are all on the beta and not the alpha.

Once the above 3 steps are done, I'll immediately create a new release for v2.2.0.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


.bootstraprc-4-default, line 10 at r2 (raw file):

# If Bootstrap version 4-alphaX use `false`
# If Bootstrap version 4-betaX use `true`
isBeta: true

remove


.bootstraprc-4-default, line 63 at r2 (raw file):

  # Reset and dependencies
  # Bootstrap dropped normalize in beta
  # Uncomment `normalize` if you still use alpha

Remove


src/bootstrap.config.js, line 130 at r2 (raw file):

  return {
    bootstrapVersion: parseInt(userConfig.bootstrapVersion, 10),
    isBeta: userConfig.isBeta || false,

Do we still need this key? what purpose is it?


src/bootstrap.styles.loader.js, line 25 at r2 (raw file):

    bootstrapRelPath,
    useFlexbox,
    isBeta,

remove


src/bootstrap.styles.loader.js, line 50 at r2 (raw file):

  }

  if (bootstrapVersion === 4 && isBeta === true) {

remove conditional for beta


Comments from Reviewable

@IvanKalinin
Copy link
Contributor Author

There are a bit more changes.

@justin808
Copy link
Member

:lgtm: Just one question in the code, that probably requires a doc note.

Please confirm all examples work and I'll merge and release.

GREAT WORK!


Reviewed 23 of 23 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


examples/basic/.bootstraprc-4-example, line 13 at r3 (raw file):

  - css-loader
  - sass-loader
  - postcss-loader

Why did this change? Just curious. Should we document this in the CHANGELOG or README? This is pretty subtle.


Comments from Reviewable

@IvanKalinin
Copy link
Contributor Author

IvanKalinin commented Aug 17, 2017

Because order matters. Postcss loader should work after sass-loader. In that order there is no errors and warnings in console.

And yes, I tried all three examples with bs4

@netbull
Copy link

netbull commented Aug 18, 2017

@justin808 any news when the PR will merged ?

@justin808
Copy link
Member

:lgtm:


Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from Reviewable

@justin808 justin808 merged commit 8a9850f into shakacode:master Aug 18, 2017
@justin808
Copy link
Member

Thanks @IvanKalinin!

@netbull
Copy link

netbull commented Aug 18, 2017

Yes, thanks @IvanKalinin

@kuongknight
Copy link

I still found issue : Argument $color of darken($color, $amount) must be a color
When using:
bootstrap-loader: 2.2.0
bootstrap: 4.0.0-beta.3
default .bootstraprc-4-default

@justin808
Copy link
Member

@kuongknight maybe submit a PR?

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.

None yet

4 participants