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

Pure css slider handles #2132

Closed
wants to merge 3 commits into from
Closed

Conversation

mmcc
Copy link
Member

@mmcc mmcc commented May 8, 2015

Opening to review the idea, do not merge

We've talked about this in the past, but in starting to work on #2117 I realized that this would simplify my work there. We've talked about this in the past, but I just wanted to formally open this conversation and get feedback.

Pros

  • Simple, general CSS (after cleanup)
  • Can remove all the slider handle update logic

Cons

  • Full width skins (such as the default 4.0 skin) become more difficult

@heff
Copy link
Member

heff commented May 8, 2015

Full width skins (such as the default 4.0 skin) become more difficult

Because you can drag it half out of the player, yeah? Can you post a screen shot of that?

@mmcc
Copy link
Member Author

mmcc commented May 8, 2015

Yes, precisely. I would need to actually fix the old skin to post a screenshot, but yes, the issue is that the slider handle would be absolutely positioned on the center of the end of the progress bar. That's great for the current skin, pretty crappy for the old skin.

I guess the kicker is how much we care about the old skin working directly out of the box, and how much we want the slider handle to be its own component.

@heff
Copy link
Member

heff commented May 8, 2015

This doesn't negate what you're saying but I think we should care a lot
about things working out of the box, so that you can do as much as possible
with the skin. In this case a YouTube-style skin has the same issue, which
might be even more relevant than making the 4.0 skin possible. That's why
I'm interested in digging more into how bad it is, or if there's anything
else we can do.

I'm not sure exactly how this fits in, but there's also the "handle" that
follows your mouse exactly (even when you're not scrubbing) and shows the
time and thumbnails (vimeo, hulu). It's in addition to the slider handle so
it's a little different issue, but may run into similar problems if we
don't want the thumbnail preview breaking out of the player.

On Thu, May 7, 2015 at 9:44 PM, Matthew McClure notifications@github.com
wrote:

Yes, precisely. I would need to actually fix the old skin to post a
screenshot, but yes, the issue is that the slider handle would be
absolutely positioned on the center of the end of the progress bar. That's
great for the current skin, pretty crappy for the old skin.

I guess the kicker is how much we care about the old skin working directly
out of the box, and how much we want the slider handle to be its own
component.


Reply to this email directly or view it on GitHub
#2132 (comment).

@mmcc
Copy link
Member Author

mmcc commented May 8, 2015

I agree, I'm just torn on how we should address that issue in particular. Note, this makes some styles easier since we don't have the weird adjusted play progress anymore. The whole scrubbing thing isn't affected at all by this change imo, since we'd still need to account for max right / left positioning.

I will say, I don't think we should let "ease of current YouTube skin implementation" affect our decision. Skins change, including the YouTube skin. The new skin preview I've got enabled doesn't have an end-to-end progress bar (you can try it out by editing a cookie), but who knows what the actual styles will end up being.

YouTube preview

@mmcc
Copy link
Member Author

mmcc commented May 8, 2015

Awkward note, those are definitely the Material UI icons (or very close), so we'll be twins there either way.

@heff
Copy link
Member

heff commented May 8, 2015

Ha, funny. I guess they ran into the same handle issue. Kind of a bummer about the icons. The more differentiation the better in the base theme, but I can live with it.

So can we:

  • Make the CSS version the default
  • Make the handle component independent of the slider, but leave it in, hidden for skins that want to enable it?

Is there any issues clicking directly on the CSS handle and dragging it? i.e. blocking the mouse tracking for the progress bar below it?

@mmcc
Copy link
Member Author

mmcc commented May 8, 2015

The problem with just leaving the handle in is that, at the moment, the handle and progress bar are dependent on each other. If we want to make the CSS version the default, I think we should break the handle out into a plugin (or something along those lines)

@heff
Copy link
Member

heff commented May 8, 2015

Yeah, that's what I meant by "make the handle independent of the slider". I still think it should be in by default, but add it as a progress bar child like the load progress bar is being added. It's no longer a piece of slider but instead specific to the progress bar. And then like the second volume control, make it hidden by default.

@heff
Copy link
Member

heff commented May 8, 2015

In #videojs chat we landed on trying to create the max-left/right for the handle using the CSS-created handle.

If that's for some reason impossible we can fall back to the separate handle component idea.

@heff heff added this to the v5.0.0 milestone May 18, 2015
@mmcc
Copy link
Member Author

mmcc commented May 18, 2015

@heff Want to take another look at this one?

Through other conversations we've decided to drop everything but css-only handles for now. If there are requests to bring back the handle classes or other needs that this makes more difficult we'll tackle them down the road. Consider that can kicked.

.video-js .vjs-slider-vertical .vjs-volume-level {
width: 0.3em;

&:before {
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to add some comments around these to make it clear that it's the handle being affected here. It's not obvious at first glance.

@heff
Copy link
Member

heff commented May 18, 2015

Man, that's a good chunk of code removed. Looks good to me.

@mmcc
Copy link
Member Author

mmcc commented May 21, 2015

Just an update, after debugging a few unrelated issues with @heff, this is working in IE8 now. However, we've uncovered a few other bugs while in IE8 that I'll open separate issues for.

The other changes that need to be made seem unrelated. Should we pull this in in the meantime?

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

Successfully merging this pull request may close these issues.

2 participants