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

Upgrade to Bootstrap 4 #856

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@davidlesieur
Copy link
Contributor

davidlesieur commented Aug 13, 2018

Notes:

  • 7 test specs are still failing. Not perfect yet, but not too bad...
  • I'm not too happy with the hardcoded px values in the stylesheet (see the .tooltip selector). Perhaps we could avoid those by using Bootstrap's variables, but that would require a stronger dependency towards Bootstrap.
  • Tooltip position is incorrect in RTL mode.
  • As I have mentioned in #689, this removes support for both LESS and Bootstrap 3.

My suggestion would be to merge this into a new branch, and continue work from there.

Pull Requests

Please accompany all pull requests with the following (where appropriate):

  • unit tests (we use Jasmine 2.x.x)
  • JSFiddle (or an equivalent such as CodePen, Plunker, etc) or screenshot/GIF with new feature or bug-fix
  • Link to original Github issue (if this is a bug-fix) #689
  • documentation updates to README file
  • examples within /tpl/index.tpl (for new options being added)
  • Passes CI-server checks (runs automated tests, JS source-code linting, etc..). To run these on your local machine, type grunt test in your Terminal within the bootstrap-slider repository directory

@davidlesieur davidlesieur changed the title Bs4 Upgrade to Bootstrap 4 Aug 13, 2018

@davidlesieur

This comment has been minimized.

Copy link
Contributor

davidlesieur commented Sep 11, 2018

Despite some failed checks, this version is pretty good and I'll soon be using it in production. I'll gladly welcome any help in fixing the remaining issues. Until it gets committed in this repo, you could submit pull requests to my development branch.

@jespirit

This comment has been minimized.

Copy link
Collaborator

jespirit commented Dec 10, 2018

It turns out when you use $.getScript() to load the JS file, the callback may be executed before the script has been fully executed. This is happening while using jQuery 3.3.1.

As a workaround, I use $.when() to wrap the jqXHR returned by $.getScript() to fully load and execute the Bootstrap-Slider library before executing the callback.

Otherwise, you'll get a deferred exception thrown for a completely different unit test unrelated to the Namespace unit tests.

 Namespace Tests
   - should always set the plugin namespace to 'bootstrapSlider'...
log: bootstrap-slider.js - WARNING: $.fn.slider namespace is already bound. Use the $.fn.bootstrapSlider namespace instead.

log: jQuery.Deferred exception: undefined is not a constructor (evaluating '$("input[data-provide=slider]")[autoRegisterNamespace]()') file:///C:/Users/Jeff/Projects/open/bootstrap-slider/_SpecRunner.html:1871:59
l@file:///C:/Users/Jeff/Projects/open/bootstrap-slider/node_modules/jquery/dist/jquery.min.js:2:58201
file:///C:/Users/Jeff/Projects/open/bootstrap-slider/node_modules/jquery/dist/jquery.min.js:2:58499 undefined
   √ should always set the plugin namespace to 'bootstrapSlider'
 Orientation Tests
   Vertical
     √ slides up when handle moves upwards
     √ slides down when handle moves downwards
 Public Method Tests
   slider constructor
     √ reads and sets the 'id' attribute of the slider instance that is created
     √ generates multiple slider instances from selector
     × reads and sets the 'min' option properly
       TypeError: undefined is not a constructor (evaluating '$("input[data-provide=slider]")[autoRegisterNamespace]()') thrown (1)
@jespirit

This comment has been minimized.

Copy link
Collaborator

jespirit commented Dec 10, 2018

RTL tests

PhantomJS 2.1.1

It turns out that, .css('right'), inside of PhantomJS will return a string as a percentage (eg. 0%) for the div.slider-selection, but returns a string as pixels (px) when the div is either the low or high track. This only seems to happen with jQuery 2.1.1.

The behaviour is consistent when using jQuery 3.3.1 in PhantomJS. .css('right') returns all pixels values.

I've tested .css('right') on latest Chrome (70), Firefox (63), and IE 11 with both jQuery 2.1.1 and jQuery 3.3.1, and the values returned are in pixels (px).

$("#rtlSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-selection").css("right")
it("slider use inversed left and right inline style", function() {
  testSlider = new Slider("#rtlSlider", {
  min: 0,
  max: 10,
  value: 5
  });

  var sliderTrackLowRight=$("#rtlSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-low").css("right");
  var sliderSelectionRight=$("#rtlSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-selection").css("right");
  var sliderTrackHighLeft=$("#rtlSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-high").css("left");

  expect(sliderTrackLowRight).toBe("0px");
  expect(sliderSelectionRight).toBe("0px");
  expect(sliderTrackHighLeft).toBe("0px");
});
RTL Tests
   rtl slider tests
     √ should be rtl by default inside an rtl wrapper
     √ rtl to false inside an rtl wrapper
     √ rtl to true inside an ltr wrapper
     × slider use inversed left and right inline style
       Expected '0px' to be '0%'. (1)
     × tooltip position must be inversed in vertical
       Expected false to be truthy. (1)
     × tooltip position can be forced in vertical
       Expected false to be truthy. (1)

PhantomJS Script

var page = require('webpage').create();

page.onConsoleMessage = function(msg) {
  console.log(msg);
};

page.open('./pull-856-rtl.html', function() {
  page.includeJs("http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js", function() {
    page.evaluate(function() {
      var sliderTrackLowRight=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-low").css("right");
      var sliderSelectionRight=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-selection").css("right");
      var sliderTrackHighLeft=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-high").css("left");

      console.log(sliderTrackLowRight);
      console.log(sliderSelectionRight);
      console.log(sliderTrackHighLeft);
    });
    phantom.exit()
  });
});

pull-856-rtl.html used in testing

<html>
  <head>
    <!-- Latest compiled and minified CSS -->
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">

    <!-- Optional theme -->
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap-theme.min.css" integrity="sha384-rHyoN1iRsVXV4nD0JutlnGaslCJuC7uwjduW9SVrLvRYooPp2bWYgmgJQIXwl/Sp" crossorigin="anonymous">

    <!-- Latest Bootstrap Slider CSS -->
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-slider/10.3.1/css/bootstrap-slider.css" integrity="sha256-hp/hrxIhTNw2AT0co+0UHMd9XUGP7wF2XwZdfoCCNEk=" crossorigin="anonymous" />
  </head>

  <body>
    <div class="container" dir="rtl">
      <input id="testSlider">
    </div>

    <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.1/jquery.min.js" integrity="sha256-wNQJi8izTG+Ho9dyOYiugSFKU6C7Sh1NNqZ2QPmO0Hk=" crossorigin="anonymous"></script>

    <!-- Latest compiled and minified JavaScript -->
    <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js" integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa" crossorigin="anonymous"></script>

    <!-- Latest Bootstrap JavaScript -->
    <script src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-slider/10.3.1/bootstrap-slider.js" integrity="sha256-dHB5CuNXZk+PUiDUge4ZJJXMRHLa1kAzGzTrSmtOZpE=" crossorigin="anonymous"></script>
    <script type="text/javascript">
      slider = new Slider('#testSlider', {
        min: 0,
        max: 10,
        value: 5
      });

      var sliderTrackLowRight=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-low").css("right");
      var sliderSelectionRight=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-selection").css("right");
      var sliderTrackHighLeft=$("#testSlider").siblings(".slider-rtl").children("div.slider-track").children("div.slider-track-high").css("left");

      console.log(sliderTrackLowRight);
      console.log(sliderSelectionRight);
      console.log(sliderTrackHighLeft);
    </script>
  </body>
</html>
@jespirit

This comment has been minimized.

Copy link
Collaborator

jespirit commented Dec 10, 2018

Resize Tests

As of jQuery 3.0+, .css('width') and .width() return fractional values for better positioning elements and animation.

See:
width(), height(), etc. shouldn't round
CSS: Make .css("width") & .css("height") return fractional values

http://jsfiddle.net/7MMhB/4/

Unit Tests

$('.slider').width(210);
dataSlider._resize();
expect($el.siblings('div.slider').find('.slider-tick-label:eq(0)').width()).toBe(53);
 Resize Tests
   Tick Labels
     × should resize the tick labels when horizontal
       Expected 52.5 to be 53. (1)
       Expected 52.5 to be 53. (2)
     × should resize the tick labels when vertical
       Expected 52.5 to be 53. (1)
       Expected 52.5 to be 53. (2)

Code

_resize: function (ev) {
  /*jshint unused:false*/
  this._state.offset = this._offset(this.sliderElem);
  this._state.size = this.sliderElem[this.sizePos];
  this._layout();
},
// if size is 210 and # of ticks is 4, then
// labelSize = 210 / 4 = 52.5
var labelSize = this._state.size / (this.options.ticks.length - 1);

@jespirit jespirit force-pushed the whiskyechobravo:bs4 branch from 49a8572 to 1a6ba7a Dec 10, 2018

@jespirit

This comment has been minimized.

Copy link
Collaborator

jespirit commented Dec 10, 2018

I rebased everything on master. All tests have passed!

@davidlesieur

This comment has been minimized.

Copy link
Contributor

davidlesieur commented Dec 10, 2018

Oh, wow! Great work!

@rovolution

This comment has been minimized.

Copy link
Collaborator

rovolution commented Dec 15, 2018

if we end up merging this i would like to release it as a major version bump (since its a pretty large change)

@rovolution

This comment has been minimized.

Copy link
Collaborator

rovolution commented Dec 15, 2018

also, make sure this line is removed from the README if this PR is merged: https://github.com/seiyria/bootstrap-slider/blame/master/README.md#L9

@davidlesieur

This comment has been minimized.

Copy link
Contributor

davidlesieur commented Dec 18, 2018

I agree, it absolutely needs a major version bump. Not everyone will be ready to upgrade to Bootstrap 4.

davidlesieur and others added some commits Jun 6, 2018

Upgrade to Bootstrap 4.
- Upgrade Node to 9 (Bootstrap requires >6).
- Get rid of LESS, Bootstrap 2, Bootstrap 3.
- Add Bootstrap 4 dependendies (jQuery, Popper.js, Autoprefixer).
Fix namespace unit tests to use $.when()
For jQuery 3.3.1, $.getScript() doesn't load AND execute the script BEFORE
executing the callback which causes the 'undefined is not a constructor'
error when running the Namespace unit tests.
Fix resize unit tests
Prior to jQuery 3.0+, .width() and .height() returned rounded values.
The methods now return fractional values for better element positioning
and for animation.

See jquery/jquery#1724
Fix RTL unit tests for jQuery 3.3.1
In PhantomJS, .css('right') in jQuery 2.1.1 returns a string as a percentage (%).
In PhantomJS, .css('right') in jQuery 3.3.1 returns a string in pixels (px).
For other browsers (Chrome, Firefox, IE), .css('right') in jQuery 2.1.1
returns a string in pixels (px).

davidlesieur added some commits Dec 18, 2018

Update readme for Bootstrap 4.
Bootstrap 4 does not support IE9.
Fix wrong CSS class for tooltip visibility.
Bootstrap 4 uses 'show' instead of 'in'.

@davidlesieur davidlesieur force-pushed the whiskyechobravo:bs4 branch from 4bb791e to 35e015f Dec 18, 2018

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