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

Remove useless v-fullscreenButtonDisplay class #156

Merged
merged 3 commits into from
Feb 22, 2025

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Feb 19, 2025

The v-fullscreenButtonDisplay component can use latest z-index instead of 2147483647.

Merge after #155 is merged.

v-fullscreenButtonDisplay class is useless, remove it.

This PR contains a:

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

Motivation / Use-Case

Improve z-index value logic and reduce size.

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 22, 2025 6:51pm

@rtritto
Copy link
Contributor Author

rtritto commented Feb 22, 2025

&.v-fullscreenButtonDisplay {
  .v-controlBar,
  .v-bigPlay {
    position: fixed;
    z-index: 8;
  }
}

Can you test if the this CSS can be removed? If not, what is the purpose?

@yoriiis
Copy link
Member

yoriiis commented Feb 22, 2025

&.v-fullscreenButtonDisplay {
  .v-controlBar,
  .v-bigPlay {
    position: fixed;
    z-index: 8;
  }
}

Can you test if the this CSS can be removed? If not, what is the purpose?

It no longer seems useful and it works without. Tested on Chrome, Firefox and Edge on fullscreen, the big play button is visible.

@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

@yoriiis I added a commit.
Now is LGTM

@rtritto rtritto changed the title Use latest z-index for v-fullscreenButtonDisplay Remove useless v-fullscreenButtonDisplay class Feb 22, 2025
@yoriiis
Copy link
Member

yoriiis commented Feb 22, 2025

@rtritto I don't know whether the CSS class v-fullscreenButtonDisplay was used by users.
Do we keep a v-fullscreen class instead as a state change only (like v-loading).
What do you think?

@rtritto
Copy link
Contributor Author

rtritto commented Feb 22, 2025

I don't know whether the CSS class v-fullscreenButtonDisplay was used by users.

You're right.

Do we keep a v-fullscreen class instead as a state change only (like v-loading).

Yes, it's better and clear.

I did the commit.

@yoriiis yoriiis enabled auto-merge February 22, 2025 18:53
@yoriiis yoriiis merged commit d5cf59d into vlitejs:main Feb 22, 2025
13 checks passed
@rtritto rtritto deleted the use-latest-z-index branch February 22, 2025 19:00
@yoriiis
Copy link
Member

yoriiis commented Feb 23, 2025

Available in release 7.2.0 🎉

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.

2 participants