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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions ui/less/overflow_menu.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@kamalmulani kamalmulani Mar 15, 2021 •
edited
Loading

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

@kamalmulani kamalmulani Mar 15, 2021 •
edited
Loading

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!

bottom: 30px;
right: 3%;
bottom: 4.4em;

/* The buttons inside the menu. */
button {
Expand Down Expand Up @@ -67,13 +67,6 @@
padding-left: 10px;
padding-right: 10px;
}

/* 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

@kamalmulani kamalmulani Mar 15, 2021 •
edited
Loading

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.

/* TODO(b/116651454): eliminate hard-coded offsets */
bottom: 15px;
}
}

/* The span elements inside the top-level overflow menu contain single lines
Expand Down Expand Up @@ -112,6 +105,17 @@
}
}

.shaka-video-container:fullscreen {
/* When in fulscreen mode, add a bottom margin to overflow menu so that it
* doesn't overlap with other buttons. */
.shaka-controls-container {
.shaka-overflow-menu {
/* TODO: eliminate hard-coded offsets */
bottom: 6em;
}
}
}

/* This is a button within each submenu that takes you back to the main overflow
* menu. */
.shaka-back-to-overflow-button {
Expand Down
5 changes: 5 additions & 0 deletions ui/less/range_elements.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@
/* The track should fill the range element. */
width: 100%;

/* Since range elements are special input elements so they should reflect,
* user interaction so whhen user hover on range element the cusror should
* be pointer. */
cursor: pointer;

/* The track should be tall enough to contain the thumb without clipping it.
* It is very tricky to make the thumb show outside the track on IE 11, and
* it is incompatible with our background gradients. */
Expand Down