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

UI improvement: fix overflow menu overlapping other controls #3220

Closed
wants to merge 1 commit into from
Closed

UI improvement: fix overflow menu overlapping other controls #3220

wants to merge 1 commit into from

Conversation

kamalmulani
Copy link

@kamalmulani kamalmulani commented Mar 14, 2021

Description

some UI improvement for the player:

  1. The range element is a special input element so changed cursor changes to pointer when it hovers over any range input element.
  2. fixed overflow menu overlapping other controls issue

Fixes #3217

Screenshots (optional)

cursor changes to pointer on range elements.

fix #1

fixed overflow menu overlapping other controls issue

Screenshot (165)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

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;
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Author

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 :

before

after these changes :

after

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.

Copy link
Contributor

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.

Copy link
Author

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.

@michellezhuogg michellezhuogg changed the title Implemented suggestions for ui improvements UI improvement: fix overflow menu overlapping other controls Mar 15, 2021
@joeyparrish
Copy link
Member

@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:

  1. Fix the pointer style
  2. Fix the positioning of the overflow menu
  3. Change fixed values to percentages and ems

If those were split up into separate PRs, it would be easier to review, test, and approve some of them more quickly.

Thanks!

@kamalmulani
Copy link
Author

sure sir, I'll do it.

@joeyparrish joeyparrish removed the gsoc label Sep 27, 2021
theodab pushed a commit that referenced this pull request Mar 23, 2022
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
joeyparrish pushed a commit that referenced this pull request Apr 21, 2022
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
joeyparrish pushed a commit that referenced this pull request Apr 21, 2022
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
joeyparrish pushed a commit that referenced this pull request Apr 21, 2022
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
@avelad avelad added this to the v4.0 milestone May 4, 2022
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overflow menus cover control panel elements
5 participants