Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Current TextTrackSettings should be a plugin to reduce weight of file #2998

Closed
Naouak opened this issue Jan 12, 2016 · 4 comments
Closed

Current TextTrackSettings should be a plugin to reduce weight of file #2998

Naouak opened this issue Jan 12, 2016 · 4 comments

Comments

@Naouak
Copy link
Contributor

Naouak commented Jan 12, 2016

Current version of TextTrackSettings has a 5KB template (Non minimifiable as it is a string contant).

A simpler version without HTML interface to configure caption appearance seems to me that would be a more usual use case of captions and would greatly reduce file size (an the file size of the minimified file).

TL;DR : remove current TextTrackSettings from current default player to trim down size and replace it with an headless lightweight version.

@gkatsev
Copy link
Member

gkatsev commented Jan 12, 2016

While, the string template -- definitely not great, and hopefully, we'll fix that sooner rather than later -- is 5kb and not really minifiable, it gzips pretty well. The full file goes from 12K to under 2.5K when gzipped. So, really, it doesn't make much of a difference. So, removing it wouldn't really make videojs any more light weight and if you're not using gzipping on your server, you really should set it up.
The utility from having it built-in is worth the very little weight that it adds to the code.
I'm going to open another issue to tracking transitioning the text-track-settings dialog from the mess that it is now into something more reasonable to track the actionable items from this issue.
Thanks.

@gkatsev gkatsev closed this as completed Jan 12, 2016
@gkatsev
Copy link
Member

gkatsev commented Jan 12, 2016

I just realized that we have #2746, so, we'll use that to track making the dialog not be a mess.

@Naouak
Copy link
Contributor Author

Naouak commented Jan 12, 2016

Size is one reason and the template is just the quick way to see there is a lot to gain.
Looking more into it, I still think that it needs to be a plugin in favor of a simpler texttracksettings.

One of the issue of the current version for example, is that it is using the dom as a storage for values. It limits possible values (if you try to set a specific color, it will generate an error because it's not a value of the select in the template), it is slower (accessing dom is a lot slower than accessing a variable in memory).

Can't we create a TextTrackSettingsUI class for the popup (which should I think be in a plugin with CaptionSettingsMenuItem) and simplify the current TextTrackSettings to be a simple store with minimum data validation ?

@gkatsev
Copy link
Member

gkatsev commented Jan 12, 2016

Yeah, as I said, what it's doing right now isn't great but it works. When we overhaul it, we'll make sure that the DOM isn't the source of truth as it is now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants