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 overlay of v-loader #155

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Feb 19, 2025

Fix #150

Move z-index of v-controlBar component on top of v-loader component.

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Additional Info

Previous z-index (5) is in the v-loader component.

Note: the z-index of v-fullscreenButtonDisplay can be changed to 8. Related PR #156

Copy link

vercel bot commented Feb 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
vlite ✅ Ready (Inspect) Visit Preview Feb 23, 2025 5:25pm

@rtritto rtritto changed the title Fix overlay of controlBar Fix overlay of v-controlBar Feb 19, 2025
@yoriiis
Copy link
Member

yoriiis commented Feb 20, 2025

The changes created a display problem in my opinion, the control bar is now displayed over the poster

image

@rtritto
Copy link
Contributor Author

rtritto commented Feb 21, 2025

I added the z-index (7) to v-poster component.
Now you will see correctly.

@yoriiis yoriiis force-pushed the fix-control-bar-overlay branch from 9b5e4f3 to aa0f0cf Compare February 22, 2025 17:52
@yoriiis
Copy link
Member

yoriiis commented Feb 22, 2025

@rtritto I've added a commit to your branch with the changes. If it's OK with you, I'll merge it.

@rtritto
Copy link
Contributor Author

rtritto commented Feb 22, 2025

@rtritto I've added a commit to your branch with the changes. If it's OK with you, I'll merge it.

I analysed current z-index values for components:

  • 1
    • control-bar
    • plugin/ima
    • plugin/sticky
  • 2
    • poster
    • plugin/subtitle
  • 3
    • big-play
    • plugin/subtitle
  • 5
    • loader

We can simply remove the z-index from loader. By this way, the loader doesn't overlay control bar, big play and other components. The whole loader logic is handled by visibility CSS property.

I did a commit, can you test it?

@yoriiis
Copy link
Member

yoriiis commented Feb 22, 2025

@rtritto I've added a commit to your branch with the changes. If it's OK with you, I'll merge it.

I analysed current z-index values for components:

  • 1

    • control-bar
    • plugin/ima
    • plugin/sticky
  • 2

    • poster
    • plugin/subtitle
  • 3

    • big-play
    • plugin/subtitle
  • 5

    • loader

We can simply remove the z-index from loader. By this way, the loader doesn't overlay control bar, big play and other components. The whole loader logic is handled by visibility CSS property.

I did a commit, can you test it?

The last commit broke several things, why did you delete several z-index? The loader is no longer displayed and when loading the poster should hide the control bar. Have you access to the demo in your local environment?

The previous commit was okay.

@rtritto
Copy link
Contributor Author

rtritto commented Feb 22, 2025

... why did you delete several z-index?

The main issue is for loader that has the z-index level higher than the control-bar and big-play.
So removing the z-index 5, the 0 will be used and control-bar and big-play are on top of loader.

The loader is no longer displayed ...

Why?
This is my test:

2025-02-22.222700.mp4

... and when loading the poster should hide the control bar

The poster is visible only when the video isn't started.
With the playback or buffering, the poster should never display.
On initial buffering (init of the player), the poster will be above the loader.
Do you agree?

On initial buffering (init of the player), should the poster be above or below the loader?

Have you access to the demo in your local environment?

I tested (I simply removed the z-index of v-loader) using this demo https://vlite.js.org

@rtritto rtritto changed the title Fix overlay of v-controlBar Fix overlay of v-loader Feb 23, 2025
@yoriiis
Copy link
Member

yoriiis commented Feb 23, 2025

@rtritto I've updated the Vercel config, approved preview deployments are now public. Check the latest deploy, accessible from the Vercel comment above.


On the latest preview, the control bar is over the poster on init. It should not.
Also, the loader needs to be over the poster for the init. You can compare on the official demo https://vlite.js.org with throttling connection or by adding the v-loader class manually.

I simply removed the z-index of v-loader

In 28a8c72, you have removed 4 z-index, check the changes in the PR.

I think moving z-index to resolve this issue is not possible. The current z-index order: (from highest to lowest)

  • loader
  • big-play (hide when loading)
  • poster
  • control
  • video

Control can't be over the loader without breaking this order which I find correct.

An alternative solution would be to probably simply add pointer-events: none to the v-loader element, to let clicks through.

Copy link

vercel bot commented Feb 23, 2025

@rtritto is attempting to deploy a commit to the Vlitejs Team on Vercel.

A member of the Team first needs to authorize it.

@rtritto
Copy link
Contributor Author

rtritto commented Feb 23, 2025

@yoriiis maybe another alternative is to change the order of the container render and fix the z-index levels.
I pushed some change.
Waiting the deploy, so that we can test.

@yoriiis
Copy link
Member

yoriiis commented Feb 23, 2025

@yoriiis maybe another alternative is to change the order of the container render and fix the z-index levels. I pushed some change. Waiting the deploy, so that we can test.

This changes break the player. I'm not fan because we have elements outside the container. which normally contains all sub elements. And it could be breaking for user.

@rtritto rtritto force-pushed the fix-control-bar-overlay branch from b65b8fa to aa0f0cf Compare February 23, 2025 17:36
@rtritto
Copy link
Contributor Author

rtritto commented Feb 23, 2025

@yoriiis Ok, I removed my latest (3) commits and restored to your commit.
I tested with previous deploy and it's LGTM.

@yoriiis
Copy link
Member

yoriiis commented Feb 23, 2025

@yoriiis Ok, I removed my latest (3) commits and restored to your commit. I tested with previous deploy and it's LGTM.

The latest Vercel did not updates with the latest force push. Will check.

Sorry but it's still not good, the loader should be displayed on top of the loader when it's the first load. I don't think there's a solution with the z-index as explained in #155 (comment).

I'll have a look later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable controls during the seek
2 participants