davidlesieur commented 5 years ago

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):

[x] 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
[x] Link to original Github issue (if this is a bug-fix) #689
[x] documentation updates to README file
[x] examples within /tpl/index.tpl (for new options being added)
[x] 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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I rebased everything on master. All tests have passed!

davidlesieur commented 5 years ago

Oh, wow! Great work!

rovolution commented 5 years ago

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

rovolution commented 5 years ago

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 commented 5 years ago

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

tailorvj commented 5 years ago

@rovolution @davidlesieur This looks interesting. How far away are you from actually releasing a Bootstrap 4 version to npm?

Kind regards, Tailor

davidlesieur commented 5 years ago

@tailorvj Unfortunately, the branch still has conflicts that need to be resolved. Anyone is welcome to help; not sure I'll have an opportunity to work on this in the coming weeks.

davidlesieur commented 5 years ago

@rovolution, can you explain the "DO NOT MERGE" label? Is it because of the branch conflicts, or some other issue?

rovolution commented 5 years ago

@davidlesieur yea this requires a rebase + merge conflict resolution + extensive QA on the Netlify preview page before I feel confident in merging it.

Also get a follow up review from @jespirit since he has kind of been leading this effort.

jsullivan commented 5 years ago

Hey folks, I think the merge conflicts are resolved with https://github.com/whiskyechobravo/bootstrap-slider/pull/1, but I haven't heard anything from @jespirit or @seiyria yet. Is anyone around to give it a quick once over?

jespirit commented 5 years ago

@jsullivan I've pulled down your bs4 branch and ran into a couple of test failures/error. Fortunately, I looked at the errors and was able to resolve them. I've rebased this branch onto master and applied the fixes and all the tests pass locally (rebase wasn't too bad).

@rovolution @seiyria I could certainly merge this pull request and release a new major version. How exactly do you handle supporting a new major version (eg. 11.0.0) but at the same time apply bug fixes/patches to the previous major version (10.6.1). Does npm support that kind of development?

jsullivan commented 5 years ago

@jespirit awesome! I'm chomping at the bit to use this so hopefully @rovolution or @seiyria will provide some direction on getting this released. @jespirit thanks so much!

seiyria commented 5 years ago

Thanks @jespirit! I don't believe npm/node will have any real support for us in that regard. Definitely, @rovolution should weigh in here as well, but my expectation would be:

the current version is "sunset" - patch releases would continue on that release path. So if we're on 10.x right now, 10.x would be forever bootstrap 3 only, and any updates to that should be on a separate branch (probably, we will have to maintain a bs3 branch)
11.x and forward is bootstrap 4
this needs to be a big notice at the top of the README

As this project is fairly simple in comparison to others, I don't expect the need to backport a ton of fixes, etc to bs3.

rovolution commented 5 years ago

@seiyria @jespirit from reading through this stackoverflow post (which albeit was written in 2014), we should be able to go back and apply patch fixes to the old major version when needed as long as we maintain an old branch we can checkout from.

We can always reference previous versions / commits since we publish tags to github on each release.

To see tags locally, type git fetch origin --tags to pull down followed by git tag

For ex: If we ever needed to go back and make a change to an older version, we could just checkout the tag for v10.6.1, checkout a new branch from that, make changes, and then publish.

I think we should update our publish scripts to account for this type of publish (like somehow check if there already exists a later version of this tag in NPM and if so, use npm publish --tag (as shown in the stackflow post) instead of just regular npm publish so that the latest pointer is not accidentally updated in NPM. Here is where we currently called publish

This article may help as well

rovolution commented 5 years ago

@davidlesieur @seiyria @jsullivan @jespirit Could we do the following?

[ ] update the README to indicate BS4 compatibility (make this somewhat noticeable)
[ ] add in that publish check i mentioned to our publish script -- see the comment above

After that, we should totally merge and release under a major bump.

rovolution commented 5 years ago

@davidlesieur @seiyria @jespirit @jsullivan I tried testing it via the netlify preview and saw errors when loading the static assetts

https://deploy-preview-856--bootstrap-slider.netlify.com/

Also, I cloned down the branch and QAed locally, and had some jankiness issues

jespirit commented 5 years ago

I will look into the static page assets not loading and check for jankiness.

jsullivan commented 5 years ago

Anybody have updates on this?

garthfield commented 4 years ago

"... but that would require a stronger dependency towards Bootstrap ..."

The plugin is literally called bootstap-slider LOL

davidlesieur commented 4 years ago

@garthfield Thank you for your penetrating vision. Still, relying on Bootstrap's Sass variables would create an additional layer of coupling.

rovolution commented 4 years ago

ok. just pulled this down and Q/Aed it locally, and everything appears to be working fine. no console errors or jankiness

rovolution commented 4 years ago

@seiyria @davidlesieur @jespirit thank you so much for your help on this effort! I am updating the CHANGELOG and README with details about future support before running the release script

rovolution commented 4 years ago

this will be under version 11.0.0 once it is released

stevenbuccini commented 4 years ago

Big thanks to everyone who made this possible!

rovolution commented 4 years ago

@stevenbuccini if you encounter any bugs or glitches with the new version, I encourage you to make PRs so we can get them reviewed and addressed immediately. thank you!

brownieboy commented 4 years ago

Github description for this repository still says "A slider control for Bootstrap 3".

seiyria commented 4 years ago

Thanks, I've updated it @brownieboy!

seiyria / bootstrap-slider

Upgrade to Bootstrap 4 #856

Pull Requests