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

Allow setting a custom incompatible video message. fixes #636 #638

Merged
merged 3 commits into from Aug 27, 2013

Conversation

Projects
None yet
3 participants
@jelbourn
Copy link
Contributor

commented Jul 12, 2013

Rough-draft pull request, tried to match the codebase as much as I could. Let me know what you think (particularly about using the global videojs options to set this message).

@jelbourn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2013

Following up on this- any comments on this pull request?

@heff

This comment has been minimized.

Copy link
Member

commented Jul 29, 2013

Sorry, currently down a rabbit hole with control bar fixes/updates.

Instead of the if/else, you could set the default as a global option that can be over ridden.
https://github.com/videojs/video.js/blob/v4.1.0/src/js/core.js#L70

'incompatibleVideoMessage': (the default message)

And then set innerHTML with that option

innerHTML: this.options()['incompatibleVideoMessage'];

At some point we need to set up video.js to allow for internationalization, so this option may change long term, but definitely a good start. Thanks!

@jelbourn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2013

Ah, didn't see global options config in core.js. Changed to set the default there and use directly.

var player = new vjs.Player(tag);

var incompatibilityMessage = player.el().getElementsByTagName('p')[0];
equal(incompatibilityMessage.textContent, 'Video no go');

This comment has been minimized.

Copy link
@heff

This comment has been minimized.

Copy link
@gkatsev

gkatsev Aug 6, 2013

Member

I think textContent is the way to go. Having an innerHTML fallback is alright.
What's the likely hood that someone will run these tests in IE8 or below?

This comment has been minimized.

Copy link
@heff

heff Aug 6, 2013

Member

Guaranteed, unfortunately. Before a release I run all the tests in the top browsers/devices, which still includes IE8.

Is there any case where innerHTML wouldn't work for this?

This comment has been minimized.

Copy link
@gkatsev

gkatsev Aug 6, 2013

Member

Oh, this is just for reading. Hm... It probably is alright to use innerHTML, though, I'd rather avoid it. It may be different depending on the exactly content of the incompatibilityMessage. Depending on the HTML structure, innerHTML and textContent will give you different results but probably not a problem in this particular case.
I would still rather have it fallback to innerHTML if textContent isn't available in the spirit of moving forward.

This comment has been minimized.

Copy link
@heff

heff Aug 6, 2013

Member

Could that be as simple as incompatibilityMessage.textContent || incompatibilityMessage.innerHTML ?

This comment has been minimized.

Copy link
@gkatsev

gkatsev Aug 6, 2013

Member

assuming IE8 doesn't yell about textContent not being available, yes.
Might need to do something like typeof incompatibilityMessage.textContent !== undefined ? incompatibilityMessage.textContent : incompatibilityMessage.innerHTML

This comment has been minimized.

Copy link
@heff

heff Aug 6, 2013

Member

Well, hang on, we're setting the value with innerHTML, so we should probably be reading it with innerHTML.
https://github.com/videojs/video.js/pull/638/files#L1L743

Otherwise we'll get different results if there's any child nodes.

For other node types, textContent returns the concatenation of the textContent attribute value of every child node, excluding comments and processing instruction nodes.

The fallback message isn't meant to be limited to just text, in fact the actual default message has links.

@heff

This comment has been minimized.

Copy link
Member

commented Aug 5, 2013

Nice, and great test. Made one comment inline.

@heff heff merged commit 20e2d4e into videojs:master Aug 27, 2013

1 check passed

default The Travis CI build passed
Details
@heff

This comment has been minimized.

Copy link
Member

commented Aug 27, 2013

Got this in, thanks. Made a few changes:

  • using innerHTML in the test, because the test breaks when you use textContent and add HTML to the fallback message
  • changed the option name to notSupportedMessage
@jelbourn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2013

Thanks for picking this up, greatly appreciated.

@jelbourn jelbourn deleted the jelbourn:636-incompatible branch Aug 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.