Slider updates - "separate" handle logic from the slider and allow for vertical or horizontal orientation #1816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
||
| handleWidth = handle.el().offsetWidth, | ||
| // Set the new bar width or height | ||
| if (this.options()['vertical']) { |
There was a problem hiding this comment.
I think this should check this.vertical() now instead of the option.
mmcc
changed the title
|
So in this PR we're still adjusting the bar location based on the handle, right? |
|
@heff Yep, that's correct. |
|
ping on this one. |
| @@ -78,11 +81,12 @@ vjs.Slider.prototype.update = function(){ | |||
| // If scrubbing, we could use a cached value to make the handle keep up with the user's mouse. | |||
| // On HTML5 browsers scrubbing is really smooth, but some flash players are slow, so we might want to utilize this later. | |||
| // var progress = (this.player_.scrubbing) ? this.player_.getCache().currentTime / this.player_.duration() : this.player_.currentTime() / this.player_.duration(); | |||
| var progress = this.getPercent(); | |||
| var handle = this.handle; | |||
There was a problem hiding this comment.
Doesn't look like this handle var is being used anymore.
|
looks good to me! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This was originally done so I could unblock myself and continue working on the base styles, so there's almost certainly room for improvement. Going ahead and opening a PR to start the conversation, but this shouldn't be pulled in yet.
The crux of this is that it allows a slider to be either horizontal or vertical without needing to use a css transform. If the vertical option is set on the slider, it will update the elements height and do the necessary calculations against the Y axis, or vice versa if the slider is horizontal.
In the process of doing this, I broke the update logic into two parts so the slider handle update is handled in its own method. This is still pretty ugly, since we have to adjust the slider bar position in the slider handle method, but it helped a bit with readability (imo).
Thoughts?