feat: svg sprite generation #1

amtins · 2024-06-09T16:35:43Z

Description

Adds SVG sprite generation to customize video.js icons.

Specific Changes proposed

The bin folder contains the cli.js file for command-line usage.
The icons folder contains all the SVG icons used to generate the sprite.
The src folder contains the svg-sprite and SVGO configuration files, as well as the script for generating the sprite.
The template folder includes the HTML file used for the preview page, along with a configuration file that can be generated via the CLI.
The vjs-svg-sprite folder contains the preview page and the default sprite.
The index.js file is only used for development and generating the default sprite.
The svg-icons.json file defines the default configuration.
The .nvmrc file to specify node version

Adds SVG sprite generation to customize video.js icons - The `bin` folder contains the `cli.js` file for command-line usage. - The `icons` folder contains all the SVG icons used to generate the sprite. - The `src` folder contains the `svg-sprite` and `SVGO` configuration files, as well as the script for generating the sprite. - The `template` folder includes the HTML file used for the preview page, along with a configuration file that can be generated via the CLI. - The `vjs-svg-sprite` folder contains the preview page and the default sprite. - The `index.js` file is only used for development and generating the default sprite. - The `svg-icons.json` file defines the default configuration.

mister-ben · 2024-06-11T11:45:44Z

Great stuff, a couple of quick thoughts

A .nvmrc would be good
I notice the output is a little smaller than what we currently have in video.js/src/images/icons.svg. Is this just better optimised?
It's not too important as long as it is documented, but should the output dir be named something more conventional like dist?

amtins · 2024-06-15T13:20:08Z

@mister-ben I've added the .nvmrc file.

I notice the output is a little smaller than what we currently have in video.js/src/images/icons.svg. Is this just better optimised?

Yes, because I've applied two optimizations, the first directly to the icons and the second directly to the generated sprite, which should further reduce the size of the final sprite.

Individual icon optimization

svg-sprite-generator/src/vjs-sprite.js

Line 41 in f9b1014

const optimizedIconSvg = optimizeTempIcon(iconPath, svgoConfig);

Sprite optimization

svg-sprite-generator/src/vjs-sprite.js

Line 53 in f9b1014

finalContent = optimizeSprite(contents, svgoCleanConfig);

It's not too important as long as it is documented, but should the output dir be named something more conventional like dist?

Yes, I can also change the destination filename to dist. While we're on the subject, do you have any suggestions for the final filename? To import the sprite, you have to do @videojs/svg-sprite-generator/dist/vjs-sprite-icons.svg. In retrospect, I have mixed feelings about the filename; I think there are still too many characters.

Regenerate `vjs-svg-sprite/index.html` to remove reference to wrong repo.

From now on, the value of `output-dir` is `dist` instead of `vjs-svg-sprite`. Access to the SVG file will be via the path `@videojs/svg-sprite-generator/dist/vjs-sprite-icons.svg` instead of `@videojs/svg-sprite-generator/vjs-svg-sprite/vjs-sprite-icons.svg`.

wseymour15

This looks great, nice work! I left a few comments and questions.

Regarding the usage of this repo, is the distributed svg file meant to replace the file here? https://github.com/videojs/video.js/pull/8260/files#diff-56a92727a19631fea08e4d8424bbf11cbf939176723287f3e2947eded506fc83

wseymour15 · 2024-12-05T17:32:05Z

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2024 amtins<[email protected]>.


We may want to update this file to align with https://github.com/videojs/video.js/blob/main/LICENSE

wseymour15 · 2024-12-05T17:32:32Z

.gitignore

@@ -0,0 +1,2 @@
+node_modules
+vjs-sprite-tmp_*


Do we want to include dist in this file?

Ideally, yes, in order to replace the player.js import with the file generated by this project. A bit like videojs-icons.css

wseymour15 · 2024-12-05T17:56:35Z

src/svgo.clean.config.js

+      params: {
+        attributes: [
+          {
+            style: 'display: none'


I am sure there is good reason, but why do we want to set this style on clean?

The main reason is that svg-sprite adds style="position:absolute". So the cleanup is to replace position:absolute with display:none. Currently, display:none is applied via JavaScript and is not present in the icons.svg file. This would allow to remove line 545.

wseymour15 · 2024-12-05T18:13:22Z

template/svg.config.file.js

+  "root-dir": "",
+  "output-dir": "",
+  "icons": {
+    "audio-description": "",


This seems bit repetitive as a similar structure exists in scg-icons.json. Would it be possible to just move those values here and remove the other file?

It may not be possible as I am unfamiliar with svg-sprite, but figured I would mention it and try to get a better understanding.

The lack of a README adds some confusion. To give a bit of context, there are two aspects to this project:

Standardize and simplify the generation of the SVG sprite used by video.js. This would allow us to remove the icons.svg file and replace the import with the file generated by this project. I was inspired by the same philosophy as the font project, which is why this project also provides the final file. So the svg-icons.json file is used to generate the sprite for this project.

Allow developers to easily replace the default icons used by video.js. All of this can be done using the command-line tool. Therefore, svg.config.file.js will serve as a configuration file template for developers to customize the icons.

wseymour15 · 2024-12-05T19:28:26Z

package.json

+    "vjs-svg-sprite": "./bin/cli.js"
+  },
+  "scripts": {
+    "build": "node index.js",


I am trying to think if there are any other scripts we may want to include here.. Would we want to include lint, or maybe clean to remove the dist?

I have added the linter as well as husky.

Thank you for taking the time to review this PR and for the comments. ❤️

I am open to any other suggestions or ideas you may have.

Run linter on pre-commit hook

The svg-sprite.config.js file is ignored because import.meta.url is not compatible with the version of javascript used by vjsstandard.

Regenerate sprite file and html page.

chore: add .nvmrc file to specify node version

f9b1014

amtins added 2 commits July 27, 2024 12:16

fix(template): footer link pointing to wrong repo

281e393

Regenerate `vjs-svg-sprite/index.html` to remove reference to wrong repo.

wseymour15 reviewed Dec 5, 2024

View reviewed changes

amtins added 16 commits December 12, 2024 14:17

chore(license): content update

99c4494

chore(package): update license type

6785d64

chore(npm): add videojs-standard

626beb6

chore(npm): add husky

c5d8867

Run linter on pre-commit hook

style(cli): comply with linter

eb3e5ed

style(index): comply with linter

d7a122a

style(svgo-clean-config): comply with linter

1d0c65e

style(svgo-config): comply with linter

f9e9548

style(svg-config-file): comply with linter

26b7345

style(vjs-sprite): comply with linter

78733aa

style(svg-config-file): comply with linter

232e33d

chore(npm): lint errors

0de1dac

chore(npm): vjsstandard ignore file

dac47dd

The svg-sprite.config.js file is ignored because import.meta.url is not compatible with the version of javascript used by vjsstandard.

style(svg-sprite-config): use quotes instead double quotes

f29f9ac

chore(npm): fix start script path

f4872bf

fix(icons): use the X icon for twitter

0845a00

Regenerate sprite file and html page.

		@@ -0,0 +1,21 @@
		MIT License

		Copyright (c) 2024 amtins<[email protected]>.

		@@ -0,0 +1,2 @@
		node_modules
		vjs-sprite-tmp_* No newline at end of file

feat: svg sprite generation #1

Are you sure you want to change the base?

feat: svg sprite generation #1

Uh oh!

Conversation

amtins commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Specific Changes proposed

Uh oh!

mister-ben commented Jun 11, 2024

Uh oh!

amtins commented Jun 15, 2024

Uh oh!

wseymour15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amtins Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amtins commented Jun 9, 2024 •

edited

Loading

amtins Jan 12, 2025 •

edited

Loading