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

fix(web): prevent navigation from overlapping video controls #9409

Closed

Conversation

Snowknight26
Copy link
Contributor

@Snowknight26 Snowknight26 commented May 12, 2024

Fixes #9405, fixes #9174.

Can't really say I like how the fix is done but there are a couple challenges here:

  • ActivityStatus component takes up real estate
    • it's not always shown
  • To accommodate for that, video controls have to be shifted up
  • Navigation areas have to account for that
  • Navigation area icon should stay centered

In my opinion there are several things that could be done differently:

  • ActivityStatus component should be implemented differently and not be floating in the bottom right. I'm no designer though so it stays where it is.
    • the video container wouldn't need any bottom margins
  • The navigation bars shouldn't have bottom margins
    • but then how do we ensure video controls are accessible?

Then there's also mobile and video aspect ratio/viewport aspect ratio to consider. At the moment this change causes the navigation area to be even smaller (since the bottom margins have been doubled):

image

danieldietzler
danieldietzler previously approved these changes May 12, 2024
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

This is a working fix, but long term this should be refactored as part of a major refactoring. I'd like to hear @alextran1502 thoughts on this though.

@zackpollard
Copy link
Contributor

I'm not a big fan of using absolute margins or paddings anywhere, this should be solvable without that and will likely just lead to more problems in the future if we did merge this. I think as you say it probably makes sense to just move the components currently overlapping with the bar to a different area of the screen where they won't overlap.

@zackpollard zackpollard dismissed danieldietzler’s stale review May 13, 2024 10:32

Probably don't want to merge this in its current state

@Snowknight26
Copy link
Contributor Author

I'm not a big fan of using absolute margins or paddings anywhere, this should be solvable without that and will likely just lead to more problems in the future if we did merge this.

I agree, though if it helps justify this PR at least until a better fix can be implemented, this does fix two regressions:

@Snowknight26 Snowknight26 deleted the navigation-video-overlay branch May 13, 2024 22:46
@Thinkscape
Copy link

Thinkscape commented May 13, 2024

@Snowknight26 Another workaround would be to not make the navigation panes be 100% height.
If they were i.e. the size of the arrow images + buffer (i.e. 50 px), they would have big enough hit-zone to be useful, but would not overlap any controls.

Here's the node from Google Photos:

image

The little arrow right -> shows up when cursor hovers roughly to the right 40% of asset, but it looks like it's handled by Javascript - which gives you much better control over bubbling and targets. In our implementation it's a :hover and an overlapping node.

@Thinkscape
Copy link

Thinkscape commented May 13, 2024

Example with video player in google photos.

Notice how the <video> has lower z-index than all the other controls, but none of the controls overlap too much of it.

image

@Snowknight26 Snowknight26 restored the navigation-video-overlay branch May 13, 2024 23:01
@Snowknight26
Copy link
Contributor Author

Whoops, deleted the branch by accident. I think that's definitely a better approach. If we're happy with something like that I can attempt to implement something similar on this PR.

@Snowknight26
Copy link
Contributor Author

Closing in favor of #9455.

@Snowknight26 Snowknight26 deleted the navigation-video-overlay branch May 14, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants