fix(svg-icons): icon size consistency #8380
Conversation
The icons have been regenerated from the svg files in `videojs/font` to ensure consistency in size. - update icons.svg file
Uses the same reference value from the font size of `font icons` to define the default height and width of `svg icons`
Uses the same size as the big-play-button font size and centers the svg icon
Uses the same size as the `volume-level` font size and centers the svg icon for both horizontal and vertical display
Uses the same size as the `play-progress` font size and removes the hover effect
Uses the same size as the `subtitles button` font size
Codecov Report
@@ Coverage Diff @@
## main #8380 +/- ##
==========================================
+ Coverage 82.68% 82.69% +0.01%
==========================================
Files 113 113
Lines 7578 7578
Branches 1821 1821
==========================================
+ Hits 6266 6267 +1
+ Misses 1312 1311 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -1,142 +1,143 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg"> | |||
| <defs> | |||
| <symbol viewBox="0 0 16 16" id="vjs-icon-play"> | |||
| <path d="M2 1v14l12-7z"></path> | |||
| <symbol viewBox="0 0 48 48" id="vjs-icon-play"> | |||
There was a problem hiding this comment.
These are much more consistent now! Just out of curiosity, how were these regenerated?
There was a problem hiding this comment.
@wseymour15 thank you for taking the time to review and comment 👍🏽
To answer your question:
It was quite a process to say the least. 🫠
My original idea was to automate everything by starting from the videojs/font project, extracting the icons directly from there and passing them to svg-screact-cli. Unfortunately, the installation of videojs/font failed, so I had to abandon the idea to avoid wasting too much time.
So I created a separate project, added the material-design-icons@v3.0.1 and svg-spreact-cli dependencies. I then retrieved each material-design-icons icon used in https://videojs.github.io/font/ one by one and put them in a directory named svg. However, videojs/font has a few custom icons which I also retrieved and put in the svg directory. I renamed each svg file to match the icon name.
The svg directory looks like:
Once this preparatory work was done, I ran the command: npx svg-spreact ./svg > icons.svg -p 'vjs-icon-'
It generates a single svg file that looks like:
Finally, I removed the use xlink:href tags, reordered the generated symbols to make the git diff a little more digestible, removed the xmlns properties, checked that everything was coherent and consistent and congratulated myself on a job well done. 😅
There was a problem hiding this comment.
Great work! When I was doing the initial development for this, I saw tools like svg-spreact-cli, but did not think it was worth the time to build out the separate SVG files. Nice to know you found a way to cut back on some of that effort!
It is also useful to know this process moving forward. If we need to ever add a new icon, we could throw it into that script, and copy the related contents into the icons.svg file.
There was a problem hiding this comment.
@wseymour15 I've created a small project to test the automation of svg generation with svg-sprite and it works quite well and is quite simple. The demo page.
Description
This PR fixes the size of svg icons using the font size of the various components as a reference size, to bring consistency between font icons and svg icons.
Specific Changes proposed
videojs/fontfont iconsto define the default height and width ofsvg iconsbig-play-button font sizeand centers the svg iconvolume-level font sizeandcenters the svg icon for both horizontal and vertical display
play-progress font sizeand removes the hover effectsubtitles button font sizeRequirements Checklist