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

Bootstrap 4 theme #72

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Bootstrap 4 theme #72

wants to merge 58 commits into from

Conversation

angel-vladov
Copy link

@angel-vladov angel-vladov commented Nov 23, 2017

This PR includes all the suggestions from Issue #60 and examples updates.

  • package.json dependencies are updated
  • All examples in docs folder are updated to use Bootstrap 4
  • Glyphicons are gone from BS4. Docs now use Fontawesome
  • Hardcoded variables that were missing in BS 3 are now present in BS 4, so those are replaced as well
  • Added the new validation classes. I've preserved the old BS 3 validation classes names as well. I find them more useful in some cases, but it might be just me.
  • All comments in the theme now point to the Bootstrap 4 instead of 3
  • IE 10, IE 9 and IE 8 are dropped from the README compatibility section. Bootstrap 4 doesn't support them.
    Bootstrap 4 does support IE 10 with issues. I haven't tested the theme in it.

You can check the example page here.

@fk
Copy link
Member

fk commented Nov 23, 2017

Yay, this is great! 🎉 🎉 🎉
Thanks a lot @angel-vladov!

Unfortunately I won't have time to review this PR until at least next week – super busy :-(
FWIW I don't have any issues with someone else merging this!
I have get up to speed with BS4 still, too – haven't used it since the early alphas.

@select2/core @select2/design Maybe we should think about creating a new repository for the BS4 version, anyway? The old version still has a few visitors/views here on GitHub; haven't checked NPM numbers yet. I haven't thought about a monorepo much, either.

@angel-vladov
Copy link
Author

I need the Bootstrap 3 theme for legacy projects I'm working on. I doubt it's just me. It will be quite an unpleasant surprise running bower update and getting a Bootstrap 4 theme in an old Bootstrap 3 project.

We should definitely have the Bootstrap 3 theme working in the foreseeable future. New repo, another branch or major version change will all work.

Copy link

@adriandelcl adriandelcl left a comment

Choose a reason for hiding this comment

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

Align left, please.

@angel-vladov
Copy link
Author

@adriandelcl - I have no idea what you mean. Can you please provide more context and where the new styles are different from the original BS3 theme so I can fix the issue?

@OLTrustNo1
Copy link

@angel-vladov Thank you so much for this PR!!!!

What @adriandelcl is saying is that on you test site (https://angel-vladov.github.io/select2-bootstrap-theme/4.0.3.html) all data displayed in the drop down list is right aligned:
image

@angel-vladov
Copy link
Author

Thanks for testing guys! Issue is fixed.

@tbelliard
Copy link

Thanks a lot for your work on this!
I just encountered a relatively minor issue: BS4 has a variable $enable-rounded (boolean) to control whether you want rounded corners are not. This variable does not affect the value of $border-radius and instead is used to test whether border radius should be applied or not. Check https://github.com/twbs/bootstrap/search?utf8=%E2%9C%93&q=enable-rounded to see how it's used in Bootstrap 4 native styles.
As you are using $border-radius directly and not testing $enable-rounded, using $enable-rounded in a bootstrap theme to control whether we want rounded corners or not is not taken into account for select2 fields.
It's easily compensated by forcing $border-radius at 0, but for full compatibility with bootstrap 4 I suppose you could adjust the use of $border-radius by adding tests for $enable-rounded in a similar way as what is done in bootstrap 4.

@angel-vladov
Copy link
Author

Interesting. Perhaps I should change it to use the border-radius mixin everywhere instead of adding $enable-rounded directly in the SCSS.

@tbelliard
Copy link

Based on the way it's used in bootstrap, the mixin definitely looks like the way to go :-)

angel-vladov and others added 4 commits December 5, 2017 02:03
the page at the original link still lists Bootstrap 3. I've used the correct URL now.
updated examples url in readme
@fsasvari
Copy link

When can we expect the release of bootstrap v4 beta theme ?

@angel-vladov
Copy link
Author

NOTE: There is Bootstrap 4 beta-3. I haven't tested my changes with it yet. Let me know if you see any issues.

@florianlacreuse
Copy link

@angel-vladov Bootstrap 4 is now officialy released (stable version): https://blog.getbootstrap.com/2018/01/18/bootstrap-4/

Can we expect any updates from your side for this PR? Thanks you for your work!

@angel-vladov
Copy link
Author

My plan is to work on this during the weekend. The PR will be updated in a day or two.

@florianajir
Copy link

Can we merge it ?

@aparakian
Copy link

Do you think this could be merged? Are the owners aware of this?

@dereuromark
Copy link

I am also curious about the status for BS4.

@florianlacreuse
Copy link

The last release was made 3+ years ago, this PR is 3 years-old, I consider this project dead. In addition, Bootstrap 5 will be released soon (currently in beta).

In our team we use the fork made by @angel-vladov (PR's author): https://github.com/angel-vladov/select2-theme-bootstrap4
He will also make a new theme for Bootstrap 5.

@angel-vladov
Copy link
Author

I've created a Bootstrap 5 version

You can find the select2 bs5 theme here
GitHub - https://github.com/angel-vladov/select2-theme-bootstrap5
NPM - https://www.npmjs.com/package/select2-theme-bootstrap5

This is a pre-release version. Please let me know if you find any bugs.

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.