-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
UI improvement: fix overflow menu overlapping other controls #3220
Conversation
1) cursor changes to pointer when hovering over range element. 2) fixed overflow menu overlapping issue.
@@ -34,8 +34,8 @@ | |||
/* Where the menu appears. */ | |||
position: absolute; | |||
z-index: 2; | |||
right: 15px; |
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.
Any specific reason we need to change the "right" property?
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.
the right property is set to static 15px, so I changed the right property to % based relative measurements (as relative measurements are more preferred over static ones) , this way the right property adapts to the container size in normal as well as fullscreen mode.
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.
We'll need to double check how it looks on different sized screens (shrink and grow the browser window, and try device emulation in Chrome, and also on large screens like TV). If we have it in px, most likely means that it wasn't working with % for one of those use cases.
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 tested these changes on the browser in all possible screen sizes, also tried device emulation on chrome ( both small and large screen sizes ) and it works perfectly fine in all cases without any issue.
for surety, you can do a check for these changes from your side.
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.
Thank you for testing!
|
||
/* If the seekbar is missing, this is positioned lower. | ||
* TODO: Solve with flex layout instead? */ | ||
&.shaka-low-position { |
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.
Any specific reason we have to remove this style? It might break the current behavior with no seekbar.
The most ideal solution might be to use the flex layout for the overflow menu to let it float above the "shaka-controls-button-panel", however it requires further investigation to make sure nothing would be broken.
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.
this style was actually present because the bottom property was set to static 30px which had to be changed when there was no seekbar , but now as I changed the bottom property to the 4em (relative measurement) it is not needed as it automatically gives required margin from bottom making sure it is above the controls even without the seekbar, I verified that the change doesn't break the current behavior. below are the images of UI without seek bar.
before these changes :
after these changes :
And you are right, the optimal and permanent solution would be to use a flex layout for the overflow menu to let it float above the control panel, but these changes should work as a good temporary solution.
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.
Thank you for verifying that!
Would you like to try out the flex layout approach, since it's not an urgent fix? We can come back to the current approach if it doesn't work.
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.
Sure sir, I'll try to implement the flex layout and see if it works.
@prophet-x, would you please split the PR into multiple? It would be easier to review and verify the CSS changes on various browsers and platforms if you would keep each PR more focused. For example, in this PR today it appears that you:
If those were split up into separate PRs, it would be easier to review, test, and approve some of them more quickly. Thanks! |
sure sir, I'll do it. |
Since range elements are special input elements, they must reflect user interaction, so when the user hovers over the range element, the cursor must be a pointer. Issue #3220
Since range elements are special input elements, they must reflect user interaction, so when the user hovers over the range element, the cursor must be a pointer. Issue #3220
Since range elements are special input elements, they must reflect user interaction, so when the user hovers over the range element, the cursor must be a pointer. Issue #3220
Since range elements are special input elements, they must reflect user interaction, so when the user hovers over the range element, the cursor must be a pointer. Issue #3220
Description
some UI improvement for the player:
Fixes #3217
Screenshots (optional)
cursor changes to pointer on range elements.
fixed overflow menu overlapping other controls issue
Type of change
Checklist: