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

Vertical slider #477

Merged
merged 6 commits into from
May 1, 2013
Merged

Vertical slider #477

merged 6 commits into from
May 1, 2013

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Apr 30, 2013

  • Change vjs.findPosX to vjs.findPosition to return both left and top.

This also removes the fallback method that doesn't use the getBoundingClientRect because according to MDN and PPK, it should be supported just about everywhere.

  • Make sliders able to handle vertical sliders in calculateDistance.

This takes a 'vertical' option on the slider bar itself. If set, it will use the top and pageY values to calculate the slider positions.

For example, to make a vertical volume slider:

  var player = videojs(document.querySelector('.video-js'), {
    'children': {
      'controlBar': {
        'children': {
          'volumeControl': {
            'children': {
              'volumeBar' : {
                vertical: true
              }
            }
          }
        }
      }
    }
  });

gkatsev added 2 commits April 30, 2013 18:59
This also removes the fallback method that doesn't use the
getBoundingClientRect because according to MDN[1] and PPK[2], it should
be supported just about everywhere.

[1] https://developer.mozilla.org/en-US/docs/DOM/element.getBoundingClientRect#Browser_compatibility
[2] http://www.quirksmode.org/dom/w3c_cssom.html#documentview
This takes a 'vertical' option on the slider bar itself. If set, it will
use the top and pageY values to calculate the slider positions.
@dmlap
Copy link
Member

dmlap commented Apr 30, 2013

That's a whole lot o' config.

@heff
Copy link
Member

heff commented Apr 30, 2013

Nice. Would you mind writing some tests for vjs.findPosition?

@gkatsev
Copy link
Member Author

gkatsev commented May 1, 2013

Good idea, I'll work on that tomorrow.
If you have any ideas of how to de-duplicate calculateDistance any further, I'm all ears.

@heff
Copy link
Member

heff commented May 1, 2013

Very nice.

The new test needs to clean itself up, either by appending the div to the qunit fixture (which auto-empties) or by removing the div manually.

Pulling in anyway, but would be good to do a follow up on that.

heff added a commit that referenced this pull request May 1, 2013
@heff heff merged commit fa79564 into videojs:master May 1, 2013
@gkatsev
Copy link
Member Author

gkatsev commented May 1, 2013

Ah, right. I think we need to do that with another test that we wrote as well.

paulcoghlan pushed a commit to paulcoghlan/video-js that referenced this pull request May 3, 2013
….js until the known issue with phantomjs is fixed.

fixes videojs#477
@gkatsev gkatsev deleted the verticalSlider branch August 12, 2015 18:49
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.

3 participants