feat: Add useSVGIcons option #8260

wseymour15 · 2023-05-02T16:45:35Z

Description

Directory of Icons: https://deploy-preview-8260--videojs-preview.netlify.app/sandbox/svg-icons.html
Player with SVG Icons enabled: https://deploy-preview-8260--videojs-preview.netlify.app/sandbox/svg-icons-enabled.html

This change adds a player option titled useSVGIcons which will replace the font icons from videojs/font with SVGs that are stored in the Players DOM. Eventually, we will want to remove all videojs/font styling, but for now we give the user the option to replace it.

To test: run this repo locally, use npm start, and go to http://localhost:9999/sandbox/svg-icons.html to see the new icons.

PR for documentation update can be found here: videojs/videojs.com#171

Specific Changes proposed

useSVGIcons player option is added, which adds a high level vjs-svg-icons-enabled class to the video element.
Created a function on the Component class called setIcon
Add the SVG sprite to the player DOM
Test pages to display all of the svg icons that are available.

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

codecov · 2023-05-02T16:46:50Z

Codecov Report

Merging #8260 (edd3448) into main (6c4997d) will increase coverage by 0.10%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             main    #8260      +/-   ##
==========================================
+ Coverage   82.57%   82.68%   +0.10%     
==========================================
  Files         112      113       +1     
  Lines        7497     7561      +64     
  Branches     1809     1818       +9     
==========================================
+ Hits         6191     6252      +61     
- Misses       1306     1309       +3

Impacted Files	Coverage Δ
...rol-bar/text-track-controls/subs-caps-menu-item.js	`90.00% <66.66%> (-10.00%)`	⬇️
src/js/control-bar/mute-toggle.js	`92.68% <75.00%> (-1.92%)`	⬇️
src/images/icons.svg	`100.00% <100.00%> (ø)`
src/js/big-play-button.js	`30.00% <100.00%> (+2.41%)`	⬆️
src/js/button.js	`64.00% <100.00%> (+1.50%)`	⬆️
src/js/clickable-component.js	`90.14% <100.00%> (+0.14%)`	⬆️
src/js/close-button.js	`93.33% <100.00%> (+0.47%)`	⬆️
src/js/component.js	`93.54% <100.00%> (+0.22%)`	⬆️
...rol-bar/audio-track-controls/audio-track-button.js	`88.23% <100.00%> (+0.73%)`	⬆️
src/js/control-bar/fullscreen-toggle.js	`95.00% <100.00%> (+0.88%)`	⬆️
... and 15 more

... and 1 file with indirect coverage changes

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

misteroneill

I like the direction! Two minor comments/thoughts.

src/js/component.js

src/js/control-bar/audio-track-controls/audio-track-button.js

src/js/component.js

test/unit/player.test.js

gkatsev · 2023-05-11T23:05:27Z

build/images.js

what does this file do and when is it run?

looks similar to the build/sandbox.js file

This was me playing around with how to include /images in the /dist directory. Updated package.json with a command to copy the folder to dist.

I believe this should make the file accessible after everything is built, but please let me know if that is not the case or if I should do this some other way :)

images/icons.svg

misteroneill

Looks great, but I had a thought about the SVG sprite URL.

misteroneill · 2023-05-12T18:57:33Z

src/css/_icons.scss

+.vjs-svg-icon {
+  display: inline-block !important;
+  background-repeat: no-repeat;
+  background-position: center;
+
+  fill: #FFFFFF;
+  height: 16px;
+  width: 16px;
+
+  // Overwrite any font content
+  &:before {
+    content: none !important;
+  }
+}


Is there a way to do this without !important? We should avoid it except in the most unusual cases.

I'm trying to think of a better way.. without it, we basically have 2 icons overlapping each other because the font icons are added through css content. A different option would be to go through everywhere those fonts are added and do a (:not) check to ensure that the content is only being added when svgs are not supposed to be used, but not sure if that is a better solution.

We can remove this once we remove the font icons entirely, but this might be the best path for now.

misteroneill · 2023-05-12T19:01:36Z

src/js/component.js

+    const useEl = document.createElementNS(xmlnsURL, 'use');
+
+    svgEl.appendChild(useEl);
+    useEl.setAttributeNS(null, 'href', `../images/icons.svg#vjs-icon-${iconName}`);


I know we talked about this path being sufficient, but the more I think about it, the more it makes sense for there to be an additional Video.js option for the URL for the SVG sprite.

It could be ../images/icons.svg by default and that should work for the Video.js CDN or unpkg or similar, but I can see use-cases where a Video.js user will want to point that to a different location.

This would also allow people to create their own icons.

const player = videojs('example', { iconsUrl: '//my-cdn.com/my-icons.svg' });

This is a very good idea! My only question is, do we expect these custom SVG sprites to have the exact same definitions of SVGs as our SVG sprite?

For icons that already exist in VJS, we expect them to have a certain id on the SVG element. If the custom sprite were to work with this implementation, it would need to have those same ids on the defs in their sprite, or else the preexisting icons won't show up.

Would it make more sense to be able to just pass that URL into the setIcon function as a param? This way, if a user wanted to overwrite an icon, they could get whatever component they want and call the function to update the icon from their sprite url. This would also allow them to avoid sprites entirely and call a file with a single SVG.

Definitely open to all ideas!

amtins · 2023-05-21T12:43:16Z

@wseymour15 this feature is absolutely awesome and is a fantastic addition to video.js! 😍 👍🏽

I have a little feedback on the responsive mode of the player which seems to be not properly supported anymore.

By setting the default font size per breakpoint it is possible to have buttons bigger or smaller depending on the screen size, see use case described here #8066 (comment)

wseymour15 · 2023-05-22T15:58:51Z

I have a little feedback on the responsive mode of the player...

Thanks @amtins, and great find! It looks like this can be addressed with some additional CSS and toying with the different breakpoints. I will be sure to include this in my updates to this feature.

EDIT: @amtins, please feel free to correct me if I am wrong here (as I certainly could be), but it sounds like the feature you are discussing is not anything in the VJS code, but the user's ability to add their own CSS to update the button size per breakpoint.

You are absolutely correct that CSS setting the font-size will not do anything on the SVG Icons. However, I believe the user will still have the ability to change the size of the buttons per breakpoint, but the CSS will look different. That's why we are making sure we have a player option to enable the SVG icons, so the user has to opt in 😄 I envision something like this would be similar CSS for icons to accomplish a similar feat:

.vjs-layout-tiny .vjs-svg-icon {
    width:  8px;
    height: 8px;
}
.vjs-layout-x-small .vjs-svg-icon {
    width:  10px;
    height: 10px;
}
....

Please let me know if I am understanding correctly, and I am not sure if the original CSS was documented anywhere, but I am happy to add some documentation to explain this!

amtins · 2023-05-26T13:01:11Z

@wseymour15 thank you for your detailed comment 👍🏽

EDIT: @amtins, please feel free to correct me if I am wrong here (as I certainly could be), but it sounds like the feature you are discussing is not anything in the VJS code, but the user's ability to add their own CSS to update the button size per breakpoint.

I may not have been precise enough in my comment, sorry.

The power of video.js is that you can redefine the size of different components using a single line of css, regardless of responsive mode. This is possible because sizes are defined in em and not in px. As a result, if a developer changes the default size of video.js, this will be reflected throughout the user interface.

Perhaps this comment is better formulated

video.js/src/css/components/_layout.scss

Lines 13 to 15 in f1558c6

    
           // Start with 10px for base font size so other dimensions can be em based and 
        
           // easily calculable. 
        
           font-size: 10px;

Demonstration

Video.js with font icons:

vjs-font.webm

Video.js with svg px:

vjs-svg-before.webm

Video.js with svg em:

vjs-svg-after.webm

wseymour15 · 2023-05-26T16:58:13Z

Perhaps this comment is better formulated

Thanks @amtins for the clarification, the videos were extremely helpful. I went ahead and updated the styling on the icons to use em instead of px so it is truly responsive to those style changes. Feel free to take a look and let me know if that appears to fix the issue!

mister-ben · 2023-05-31T16:22:21Z

Nit, but the different volume icons don't seem quite in line with each other, the speaker is larger in the medium and low states.

The CC icon seems much more legible at a smaller size now compared to the font.

wseymour15 · 2023-05-31T19:13:36Z

Nit, but the different volume icons don't seem quite in line with each other, the speaker is larger in the medium and low states.

@mister-ben Agreed, those did look a bit inconsistent. This is because these were a mix of the Font Awesome and Material UI Icons. Ideally, I wanted to go with all of the Font Awesome Icons here, but some of them required a PRO account.

With my most recent commit, I decided to use Material UI Icons for all of the volume SVGs. I had to add a little extra CSS because those icons filled less of the viewBox then the other icons and appeared a bit too small.

BONUS: These icons require less data than the Font Awesome ones :)

mister-ben · 2023-06-01T07:35:52Z

This brings back an issue on Chrome in Chinese that the slider handles are misplaced. That was fixed with the font icons in #7990.
It's to do with the browser setting a minimum font size valuewhen the OS or app langauge is Chinese. This complicates using em units for positioning. Conceivably users who set a minium font size for accessibility reasons would also be affected. Setting the page to Chinese or user language preference doesn't change that minium setting, so does not reproduce.

On the other hand it fixes the placement of the slider on the veritcal slider, which is unresolved with the font. #8092

wseymour15 · 2023-06-01T17:03:54Z

This brings back an issue on Chrome in Chinese that the slider handles are misplaced.

Thanks for pointing this out @mister-ben ! I played around with a combination of the two solutions, and this looks better both in Chinese and non-Chinese browsers.. let me know if you agree!

src/css/components/_progress.scss

src/css/components/_volume.scss

mister-ben · 2023-06-01T17:49:41Z

Your last changes still didn't look right on my machine. Do my suggestions look right to you? Interesting and frustrating if we're getting different results.

misteroneill

Looks great. Two small questions

src/css/_icons.scss

src/js/player.js

mister-ben · 2023-06-08T11:32:56Z

The inconsistencies in these icons are bothering me:

The fullscreen exit icon is more rounded than fullscreen enter
The new CC icon is better than the old as it's much more legible, is there an matching icon for HD?
The play and pause icons are rounded, but most other icons are not, e.g. the arrows on replay, next item, download. I'd be minded to keep the simple, straight material icons as the default set.

Could we reduce the number of icons? I know why they're in the font, but I think the Facebook etc social icons in particular ought not to be included as they are not used by the core player. Instead, we should have a way for a plugin to supplement icons, some sort of registerIcon().

The size of some icons in the source seem inefficient. Are we happy with how rollup-plugin-svg is optimising them in the build?

My major concern is how we will allow users to change the icons and how easily, for example to a rounded set. As this is experimental and opt-in, that could be addressed in a later PR as long as the current approach isn't locking that out too much.

misteroneill · 2023-06-08T15:13:13Z

Great points, @mister-ben. I propose that we change the name to experimentalSvgIcons to indicate that it's a work in progress. We can review the specific icons used and customization paths in the future (even if it's just a new sample/sandbox page).

wseymour15 · 2023-06-09T16:51:40Z

Thanks @mister-ben, I greatly appreciate this feedback.

The fullscreen exit icon is more rounded than fullscreen enter

I do see that this is slightly off.. I could not find these icons at all in material UI, and the matching fullscreen exit icon required a Font Awesome pro account to download.

The new CC icon is better than the old as it's much more legible, is there an matching icon for HD?

Same here.. there IS a better HD icon in Font Awesome.. that requires a pro account.

The play and pause icons are rounded, but most other icons are not.

I made an update to make these icons align more closely.

I believe that making the icons more consistent can probably come with future work. I think it would be to our benefit to have access to a Font Awesome Pro account. Material UI doesn't quite have everything we need, and many of the "ideal" icons are locked in Font Awesome behind a pro account.

Could we reduce the number of icons?

Great point. I know we do use a few of these social icons on the Brightcove side, but they are not a part of video.js anywhere. I'm leaning towards keeping the same icons as the past for now. However I am all for taking the time later to determine which icons would and would not be valuable to keep.

The size of some icons in the source seem inefficient. Are we happy with how rollup-plugin-svg is optimising them in the build?

Most of these icons come straight from material UI or Font Awesome, I am sure there are ways to make some of them smaller, but I think for now we should probably just keep them as is.

For the build, this was a talking point on the Brightcove side initially, and we did some testing. We are satisfied with how much larger the build has become after the change, and assume once we remove the fonts entirely, it should be smaller than before. Here is some comparison between the builds before and after.

My major concern is how we will allow users to change the icons and how easily..

This sounds related to your idea of a registerIcon() function, which I think would bring a lot of value to VJS. It could be a bit tricky especially if we are trying to avoid things like using innerHTML or trying to avoid Data URIs. However, it would probably be possible to pass in a URL to an SVG file, manipulate it and add it to the DOM.

I also think that there will currently be ways the user could add icons by customizing their own player, it would just not be as simple as a function call.

Sorry for the story book 😅 All in all, I think we are at a pretty good spot to move forward with what we have, and then iterate with future improvements.

wseymour15 · 2023-06-09T17:01:16Z

Great points, @mister-ben. I propose that we change the name to experimentalSvgIcons to indicate that it's a work in progress. We can review the specific icons used and customization paths in the future (even if it's just a new sample/sandbox page).

Totally agree. Updated the current implementation to change the option name, as well as updated the docs in my other PR.

feat: Add useSVGIcons option

11862fb

wseymour15 self-assigned this May 2, 2023

wseymour15 mentioned this pull request May 2, 2023

docs: Add useSVGIcons option videojs/videojs.com#171

Merged

wseymour15 added 2 commits May 2, 2023 14:26

update path to sandbox icons page

47f743f

update to using inline SVG and setIcon component functionality

e9c36e0

wseymour15 marked this pull request as draft May 10, 2023 21:00

misteroneill reviewed May 11, 2023

View reviewed changes

src/js/component.js Outdated Show resolved Hide resolved

src/js/control-bar/audio-track-controls/audio-track-button.js Outdated Show resolved Hide resolved

mister-ben reviewed May 11, 2023

View reviewed changes

src/js/component.js Outdated Show resolved Hide resolved

wseymour15 added 4 commits May 11, 2023 11:42

add tests and update setIcon calls

8ece4f3

update sandbox page with inline svg

eb57850

minor documentation updates

79bbc2f

add player example to sandbox

de322e2

gkatsev reviewed May 11, 2023

View reviewed changes

wseymour15 added 3 commits May 12, 2023 10:26

add images to dist directory

cf09d5d

small doc updates and additional test

a898a68

sandbox fix

144907a

wseymour15 marked this pull request as ready for review May 12, 2023 16:57

misteroneill reviewed May 12, 2023

View reviewed changes

wseymour15 added 6 commits May 15, 2023 10:16

move images directory

4997916

update sandbox and npm commands to build images in dist

f150ddc

update rollup config to include svg sprite as data uri

5039685

remove images directory from dist

a4352a4

add images to netlify correctly

a3ff4c9

fix netlify with images

92a14d8

wseymour15 added 2 commits May 23, 2023 15:22

add sprite to dom and use DOMParser

d9f1c55

Merge branch 'main' into feat-add-svg-icons

20c7835

make svg icons responsive to font-size

9c52208

small doc update for set icon

4575c88

dzianis-dashkevich approved these changes May 30, 2023

View reviewed changes

remove unnecessary rollup plugin

0e465bd

minor style change to volume icons

9c90924

Merge branch 'main' into feat-add-svg-icons

5076cdf

css update to account for Chinese font sizing

c65109b

mister-ben reviewed Jun 1, 2023

View reviewed changes

src/css/components/_progress.scss Outdated Show resolved Hide resolved

mister-ben reviewed Jun 1, 2023

View reviewed changes

src/css/components/_volume.scss Outdated Show resolved Hide resolved

additional change for chinese sliders

7643e8f

misteroneill reviewed Jun 6, 2023

View reviewed changes

src/css/_icons.scss Outdated Show resolved Hide resolved

src/js/player.js Outdated Show resolved Hide resolved

wseymour15 added 2 commits June 6, 2023 16:57

small css change and svg sprite failure fix

357adf1

Merge branch 'main' into feat-add-svg-icons

a63a20c

misteroneill approved these changes Jun 7, 2023

View reviewed changes

use experimental naming and minor icon improvements

edd3448

wseymour15 merged commit 6fc0dc7 into main Jun 12, 2023

wseymour15 deleted the feat-add-svg-icons branch June 12, 2023 18:31

amtins mentioned this pull request Sep 20, 2023

Bundle size change from 8.3.0 to 8.5.2 #8443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add useSVGIcons option #8260

feat: Add useSVGIcons option #8260

wseymour15 commented May 2, 2023 •

edited

Loading

codecov bot commented May 2, 2023 •

edited

Loading

misteroneill left a comment

gkatsev May 11, 2023

gkatsev May 11, 2023

wseymour15 May 12, 2023 •

edited

Loading

misteroneill left a comment

misteroneill May 12, 2023

wseymour15 May 12, 2023

misteroneill May 12, 2023

wseymour15 May 15, 2023

amtins commented May 21, 2023

wseymour15 commented May 22, 2023 •

edited

Loading

amtins commented May 26, 2023 •

edited

Loading

wseymour15 commented May 26, 2023

mister-ben commented May 31, 2023

wseymour15 commented May 31, 2023 •

edited

Loading

mister-ben commented Jun 1, 2023

wseymour15 commented Jun 1, 2023 •

edited

Loading

mister-ben commented Jun 1, 2023

misteroneill left a comment

mister-ben commented Jun 8, 2023

misteroneill commented Jun 8, 2023

wseymour15 commented Jun 9, 2023 •

edited

Loading

wseymour15 commented Jun 9, 2023

feat: Add useSVGIcons option #8260

feat: Add useSVGIcons option #8260

Conversation

wseymour15 commented May 2, 2023 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

codecov bot commented May 2, 2023 • edited Loading

Codecov Report

misteroneill left a comment

Choose a reason for hiding this comment

gkatsev May 11, 2023

Choose a reason for hiding this comment

gkatsev May 11, 2023

Choose a reason for hiding this comment

wseymour15 May 12, 2023 • edited Loading

Choose a reason for hiding this comment

misteroneill left a comment

Choose a reason for hiding this comment

misteroneill May 12, 2023

Choose a reason for hiding this comment

wseymour15 May 12, 2023

Choose a reason for hiding this comment

misteroneill May 12, 2023

Choose a reason for hiding this comment

wseymour15 May 15, 2023

Choose a reason for hiding this comment

amtins commented May 21, 2023

wseymour15 commented May 22, 2023 • edited Loading

amtins commented May 26, 2023 • edited Loading

Demonstration

wseymour15 commented May 26, 2023

mister-ben commented May 31, 2023

wseymour15 commented May 31, 2023 • edited Loading

mister-ben commented Jun 1, 2023

wseymour15 commented Jun 1, 2023 • edited Loading

mister-ben commented Jun 1, 2023

misteroneill left a comment

Choose a reason for hiding this comment

mister-ben commented Jun 8, 2023

misteroneill commented Jun 8, 2023

wseymour15 commented Jun 9, 2023 • edited Loading

wseymour15 commented Jun 9, 2023

wseymour15 commented May 2, 2023 •

edited

Loading

codecov bot commented May 2, 2023 •

edited

Loading

wseymour15 May 12, 2023 •

edited

Loading

wseymour15 commented May 22, 2023 •

edited

Loading

amtins commented May 26, 2023 •

edited

Loading

wseymour15 commented May 31, 2023 •

edited

Loading

wseymour15 commented Jun 1, 2023 •

edited

Loading

wseymour15 commented Jun 9, 2023 •

edited

Loading