fix(control-bar): align center #7990

KangXinzhi · 2022-11-03T05:55:33Z

Description

[Please describe the change as necessary.
The bar is not centered in video control section
issues7989

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chrome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

welcome · 2022-11-03T05:55:35Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov · 2022-11-03T07:13:58Z

Codecov Report

Merging #7990 (e2faeca) into next (fa57723) will decrease coverage by 0.97%.
The diff coverage is 95.00%.

❗ Current head e2faeca differs from pull request most recent head fd4dbe9. Consider uploading reports for the commit fd4dbe9 to get more accurate results

@@            Coverage Diff             @@
##             next    #7990      +/-   ##
==========================================
- Coverage   81.95%   80.98%   -0.98%     
==========================================
  Files         110      116       +6     
  Lines        7333     7472     +139     
  Branches     1767     1817      +50     
==========================================
+ Hits         6010     6051      +41     
- Misses       1323     1421      +98

Impacted Files	Coverage Δ
src/js/player.js	`88.37% <50.00%> (-1.39%)`	⬇️
src/js/event-target.js	`100.00% <100.00%> (+4.25%)`	⬆️
src/js/extend.js	`85.71% <100.00%> (ø)`
src/js/tech/html5.js	`67.29% <100.00%> (+0.34%)`	⬆️
src/js/tech/tech.js	`83.00% <100.00%> (-0.72%)`	⬇️
src/js/tracks/text-track.js	`93.12% <100.00%> (+0.17%)`	⬆️
src/js/utils/events.js	`81.00% <100.00%> (+0.09%)`	⬆️
src/js/poster-image.js	`65.71% <0.00%> (-9.90%)`	⬇️
src/js/utils/obj.js	`68.75% <0.00%> (-6.77%)`	⬇️
src/js/live-tracker.js	`93.43% <0.00%> (-5.00%)`	⬇️
... and 37 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mister-ben

This causes the opposite issue for me: the handle is too high. There's been a few reports of this before #7004 which users have fixed with some CSS. It seems specific to Chinese OS/page. I wonder if there's something different about font rendering with Chinese characters which affects em units.

KangXinzhi · 2022-11-03T08:27:20Z

too high. There's been a few reports of th

When I open the videoJS official website, I have the same problem. and it has not any Chinese.

mister-ben · 2022-11-03T19:29:09Z

I can replicate what you see if I run Chrome in Chinese Traditional on MacOS - the application rather than the page language is apparently what matters. I note the handle is slightly larger.

Removing those two top rules and replacing them with line-height: .35em; seems to work well with the browser in English, Chinese, Arabic, Hindi, Japanese on Mac Chrome, Safari, Firefox and Edge. Would need some testing on Windows - if it works there two, it looks like a solution. It should be merged into next rather than main - it could be a breaking change for custom styles.

mister-ben · 2022-11-04T18:16:54Z

In this codepen, the first player uses the styling we currently have, and the second uses line-height instead of top. On all MacOS browsrs I've tried, the slider handles are in the correct place regarless of OS/app language for the second player.

I haven't tried on Windows as the standard edition of Windows doesn't let you change the OS/app language. If @KangXinzhi or anyone else could try this on Chinese (or other non-Latin script) Windows browsers it would be useful.

https://codepen.io/mister-ben/full/MWXeOKr

KangXinzhi · 2022-11-05T16:05:16Z

In this codepen, the first player uses the styling we currently have, and the second uses line-height instead of top. On all MacOS browsrs I've tried, the slider handles are in the correct place regarless of OS/app language for the second player.

I haven't tried on Windows as the standard edition of Windows doesn't let you change the OS/app language. If @KangXinzhi or anyone else could try this on Chinese (or other non-Latin script) Windows browsers it would be useful.

https://codepen.io/mister-ben/full/MWXeOKr

It is great when I try this on Chinese Windows browsers！

mister-ben · 2022-11-06T09:30:39Z

src/css/components/_progress.scss

+.video-js .vjs-progress-control:hover .vjs-play-progress:before{
+  top: -0.333333333333333em;
+}
+


Suggested change

.video-js .vjs-progress-control:hover .vjs-play-progress:before{

top: -0.333333333333333em;

}

mister-ben · 2022-11-06T09:31:16Z

src/css/components/_progress.scss

@@ -79,8 +83,7 @@
    font-size: 0.9em;
    position: absolute;
    right: -0.5em;
-    top: -0.333333333333333em;
-    z-index: 1;
+    top: -0.45em;


Suggested change

top: -0.45em;

line-height: .35em;

z-index: 1;

mister-ben · 2022-11-06T09:31:34Z

src/css/components/_volume.scss

@@ -157,7 +157,7 @@

  // Volume handle
  &:before {
-    top: -0.3em;
+    top: -0.45em;


Suggested change

top: -0.45em;

line-height: .35em;

mister-ben

These suggested changes are as my codpen example.

mister-ben · 2022-11-07T16:05:44Z

This could be a breaking change for custom skins, so should be in a major release - v8.x should be very soon now. Could you rebase agains the next branch?

KangXinzhi · 2022-11-08T02:53:20Z

This could be a breaking change for custom skins, so should be in a major release - v8.x should be very soon now. Could you rebase agains the next branch?

that's ok!

misteroneill

Took a quick look and seems to work nicely. Thank you @KangXinzhi!

welcome · 2022-11-08T14:18:13Z

Congrats on merging your first pull request! 🎉🎉🎉

…istent (#7990) Fixes #7989 BREAKING CHANGE: This changes how slider handles are styled, so custom skins that are targeting them may need to change.

…istent (videojs#7990) Fixes videojs#7989 BREAKING CHANGE: This changes how slider handles are styled, so custom skins that are targeting them may need to change.

mister-ben requested changes Nov 3, 2022

View reviewed changes

KangXinzhi force-pushed the fix-video-control-bar-center branch from fe8a1dd to f182580 Compare November 6, 2022 09:06

mister-ben reviewed Nov 6, 2022

View reviewed changes

mister-ben requested changes Nov 6, 2022

View reviewed changes

mister-ben changed the base branch from main to next November 6, 2022 09:39

mister-ben changed the base branch from next to main November 6, 2022 09:39

fix(control-bar): align center

fd4dbe9

KangXinzhi force-pushed the fix-video-control-bar-center branch from f182580 to fd4dbe9 Compare November 8, 2022 02:48

KangXinzhi changed the base branch from main to next November 8, 2022 02:48

mister-ben approved these changes Nov 8, 2022

View reviewed changes

mister-ben added major 8.x needs: LGTM Needs one or more additional approvals labels Nov 8, 2022

misteroneill approved these changes Nov 8, 2022

View reviewed changes

misteroneill merged commit 2b55958 into videojs:next Nov 8, 2022

mister-ben mentioned this pull request Nov 11, 2022

The bar is not centered in video control section #7989

Closed

mister-ben mentioned this pull request Jan 18, 2023

about volume location not at centre #8077

Closed

mister-ben mentioned this pull request Jan 24, 2023

about volume location not at centre at controlBar: {volumePanel: {inline: false}}, at 8.0.3 in chinese #8092

Open

misteroneill removed the needs: LGTM Needs one or more additional approvals label May 23, 2023

mister-ben mentioned this pull request Jun 1, 2023

feat: Add useSVGIcons option #8260

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(control-bar): align center #7990

fix(control-bar): align center #7990

KangXinzhi commented Nov 3, 2022

welcome bot commented Nov 3, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

mister-ben left a comment

KangXinzhi commented Nov 3, 2022

mister-ben commented Nov 3, 2022

mister-ben commented Nov 4, 2022

KangXinzhi commented Nov 5, 2022 •

edited

Loading

mister-ben Nov 6, 2022

mister-ben Nov 6, 2022

mister-ben Nov 6, 2022

mister-ben left a comment

mister-ben commented Nov 7, 2022

KangXinzhi commented Nov 8, 2022

misteroneill left a comment

welcome bot commented Nov 8, 2022

	.video-js .vjs-progress-control:hover .vjs-play-progress:before{
	top: -0.333333333333333em;
	}

fix(control-bar): align center #7990

fix(control-bar): align center #7990

Conversation

KangXinzhi commented Nov 3, 2022

Description

Specific Changes proposed

Requirements Checklist

welcome bot commented Nov 3, 2022

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

mister-ben left a comment

Choose a reason for hiding this comment

KangXinzhi commented Nov 3, 2022

mister-ben commented Nov 3, 2022

mister-ben commented Nov 4, 2022

KangXinzhi commented Nov 5, 2022 • edited Loading

mister-ben Nov 6, 2022

Choose a reason for hiding this comment

mister-ben Nov 6, 2022

Choose a reason for hiding this comment

mister-ben Nov 6, 2022

Choose a reason for hiding this comment

mister-ben left a comment

Choose a reason for hiding this comment

mister-ben commented Nov 7, 2022

KangXinzhi commented Nov 8, 2022

misteroneill left a comment

Choose a reason for hiding this comment

welcome bot commented Nov 8, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

KangXinzhi commented Nov 5, 2022 •

edited

Loading