search

videojs / font

Icon font used for Video.js
https://videojs.github.io/font/
Apache License 2.0
60 stars 79 forks source link

Bug/fix custom icon not loaded #32

Closed 199911 closed 6 years ago

199911 commented 6 years ago

The icon inside merged config does not assigned to the font generator

thijstriemstra commented 6 years ago

ping @gkatsev

gkatsev commented 6 years ago

Thanks for the ping @thijstriemstra. I'll take a look soon.

gkatsev commented 6 years ago

@199911 hey, can you provide an example of where we currently fail? Using the example grunt --custom-json=./lib/custom.json, I can see that visibility is being added properly. I'm not opposed to the change but would like to understand a bit more about the issue before merging.

199911 commented 6 years ago

Confirmed the current font generator version works, but it is a little bit tricky implementation

In current version, icons always point to the same array. When merging custom configs, no new array of icons is created, only the content of original array is updated.

Even icons is "not updated" after merging custom config, we can still find the new icon

PS: I got a bug when I am trying to add a new feature: remove the default icon by setting icon.svg to null. When i come through this piece of tricky code, I thought it is buggy and fire this PR.

gkatsev commented 6 years ago

Oh, didn't even think about removing icons. Let me try it out but this can probably go in. Thanks!

gkatsev commented 6 years ago

Merged. Thanks @199911!

thijstriemstra commented 5 years ago

This broke previous behavior where it excluded the default video.js icons. I now get all video.js + my custom icons, although I don't need the default video.js icons since they're already loaded by video.js itself.

Opened #36