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

Name techs #1786

Closed
gkatsev opened this issue Jan 8, 2015 · 6 comments
Closed

Name techs #1786

gkatsev opened this issue Jan 8, 2015 · 6 comments

Comments

@gkatsev
Copy link
Member

gkatsev commented Jan 8, 2015

It'll be very helpful if tech components were named as such: "Tech {{tech name}}". It'll make debugging easier when you just have a component with a name property set to null.

@heff
Copy link
Member

heff commented Jan 8, 2015

I'm not following. Can you give an example?

@gkatsev
Copy link
Member Author

gkatsev commented Jan 8, 2015

If you print out a tech in the console, it's name_ property is set to null. Only components like BigPlayButton have name_ properties.
Or if you call .name() on them you get 'null'.

@gkatsev
Copy link
Member Author

gkatsev commented Jan 8, 2015

player.tech.name() // using non-minified videojs
// null
player.bigPlayButton.name()
// "bigPlayButton"

Would be nicer if we got:

player.tech.name() // using non-minified videojs
// "Tech html5"

instead.

@heff
Copy link
Member

heff commented Jan 8, 2015

Are you saying specifically give all tech names like "Tech html5", or that in the case where a tech name is null, the name would be "Tech [nothing]"?

The component names are used for option setting and parent object references, so I don't know if we want to be prefixing the names.

player.controlBar // <-- component name
videojs(id, { bigPlayButton: false }) // <-- component name

Could we just make sure the name property gets set for techs?

@gkatsev
Copy link
Member Author

gkatsev commented Jan 8, 2015

yeah, even if name returns 'tech' that give me a lot more info. I was debugging an issue earlier where a tech had no associated el() and was erroring out on timeupdate. Took me a long time to realize that the component that the timeupdate was triggered on was a tech.

@heff
Copy link
Member

heff commented Jan 12, 2015

Cool. I think because of the point about how the name is used, adding a name to the techs by default would be the best approach. e.g.

vjs.Html5.prototype.name_ = 'html5';

gkatsev added a commit to gkatsev/video.js that referenced this issue Jan 11, 2017
This helps with debugging to know what a component's name is.
We try to look up the tech's name via the constructor's name property,
otherwise, we set it to 'Unknown Tech'. Can be overridden by setting
`this.name_` after calling `super()` in the constructor.

Fixes videojs#1786.
gkatsev added a commit that referenced this issue Jan 11, 2017
This helps with debugging to know what a component's name is.
We try to look up the tech's name via the constructor's name property,
otherwise, we set it to 'Unknown Tech'. Can be overridden by setting
`this.name_` after calling `super()` in the constructor.

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

No branches or pull requests

2 participants