-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
fix: time tooltip truncated #8527
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8527 +/- ##
==========================================
+ Coverage 83.45% 83.88% +0.42%
==========================================
Files 113 113
Lines 7596 8834 +1238
Branches 1827 2211 +384
==========================================
+ Hits 6339 7410 +1071
- Misses 1257 1424 +167 ☔ View full report in Codecov by Sentry. |
@harisha-swaminathan - I believe the solution at #7308 is better, as I tried to explain here: #8273. The reasons why that change was reverted imo do not apply any longer. |
@harisha-swaminathan, @phloxic I'm not quite sure I understand the problem as a whole. What I do understand (but maybe I'm wrong) is that we want to prevent the To do this, the only parameters we need are:
With these 3 parameters, we should be able to keep the timeTooltip inside the seek bar. In this case, we could do something like #8530 |
@amtins - I just rebased what I am using onto main here. Demo v8.3.0 without the change here |
@phloxic thank you for these clarifications. I'm currently going through all the issues (and there are a lot) to try and understand the initial problem. |
Certainly a good idea, see #7308 (comment). I was too focused on the out of bounds on the right problem (which does still exist though), sorry. |
@amtins @phloxic I set up demo players to show
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a simple fix that addresses the issue, helps define expected UI behavior, and doesn't prevent a deeper refactor in the future (if that's something we'd like to invest effort into).
Both in https://codepen.io/hswaminathan/pen/xxBOVLZ and https://codepen.io/hswaminathan/pen/VwRjmBW you can see the tooltip staying within the left boundary of the control bar or the player(which imo is the correct behavior), but stuck on the right by the boundary of the progress bar. It might still worth looking into #8530 Also, when the cursor is dragged the current time tooltip does weird jumps on both ends. In my tests this could be remedied by setting enableSmoothSeeking. |
@phloxic I've updated the PR to set the tooltip boundary within the progress bar on both sides for the sake of consistency. Looks like #8530 too limits the tooltip within the progress bar instead of the control bar. Also, I observed that in https://phloxic.productions/test/videojs/ttt/v8-dev.html, the tooltip overflows in the left side. |
Yes, that's consistent. But isn't it kind of ironic (or inconsistent) from a viewer's perspective to reduce the room for tooltip display specifically for the use cases of tooltips which are enlarged, e.g., thumbnail previews etc. or, indeed, scaling the player? It's technically correct, but in my personal opinion a step backwards from the audience's point of view. I don't think there are a lot of complaints about the tooltip sticking out of bounds of the progress bar. I'm reasonably certain that I would notice the general behaviour change (and complain). But then I might be the only one ;-) Having said that I'm obviously fine with conceding that @roman-bc-dev's argument may be the realistic way forward for the project.
Yes, in the second example.
Yes, scaling the player is still an issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @roman-bc-dev that this is an overall improvement even with existing limitations. Great discussion and thanks for the feedback @phloxic!
Description
spaceRightOfPoint is always
NaN
for mouse time display (unlike playProgressBar's time display) because the seekbarRect (dom.findPosition(seekbar)) does not have aright
property.And since spaceRightOfPosition is always
NaN
, when the mouse tool tip is moved close to edge of the right side player, it's position is not adjusted as it is on the left and so it get's cut off.Specific Changes proposed
The proposed change is to ignore the gap between the right edge of the
SeekBar
and the player (playerRect.right - seekBarRect.right
) in such cases.Requirements Checklist