Remove deprecation for a Button created with a non-button element #3828
Conversation
| @@ -26,24 +26,14 @@ class Button extends ClickableComponent { | |||
| * @method createEl | |||
| */ | |||
| createEl(tag = 'button', props = {}, attributes = {}) { | |||
| if (tag !== 'button') { | |||
| throw new Error('Buttons must be created with a "button" tag.'); | |||
There was a problem hiding this comment.
why do we even allow a tag to be passed in?
There was a problem hiding this comment.
Because createEl does by default. In my opinion, it would be strange to have a bunch of createEl methods, but only one that has a different signature.
There was a problem hiding this comment.
https://github.com/videojs/video.js/blob/master/src/js/tracks/text-track-display.js#L165
https://github.com/videojs/video.js/blob/master/src/js/control-bar/live-display.js#L38
https://github.com/videojs/video.js/blob/master/src/js/control-bar/control-bar.js#L39
https://github.com/videojs/video.js/blob/master/src/js/loading-spinner.js#L19
https://github.com/videojs/video.js/blob/master/src/js/menu/menu-button.js#L114
https://github.com/videojs/video.js/blob/master/src/js/popup/popup-button.js#L62
https://github.com/videojs/video.js/blob/master/src/js/player.js#L492
Most createEl calls that we have don't seem to follow this format (other than base classes) like clickable-component and component basically.
There was a problem hiding this comment.
I think the reason for this was originally backwards compatibility. I wonder if we can now just ignore the passed in tag and and always create a button. We'd want to still log the warning, though. So, maybe just throwing is fine :)
@OwenEdwards thoughts?
There was a problem hiding this comment.
Well, we definitely want to accept props and attributes, I think, for custom buttons. But I suppose changing the signature isn't the worst thing in the world.
There was a problem hiding this comment.
making it would be good to not throw? Doesn't seem like a fatal error to me, but I could be persuaded either way.
There was a problem hiding this comment.
Yeah. I could see just calling log.error in this case.
There was a problem hiding this comment.
There were still a couple of components which are clickable <div>s instead of <button>s - one of them was the poster image (I don't remember the other). I'd been discouraged from making DOM changes, so I made this split to maintain DOM backward-compatibility while fixing problems in the JavaScript and allowing menu buttons to work correctly. Ultimately (in v6.0?) the clickable-component and the button components should be merged again, and the option to pass a tag removed.
There was a problem hiding this comment.
We are going to move this conversation over to: #3837
misteroneill
force-pushed
the
buttons-are-buttons
branch
from
5093d8c to
f0ab9a9
Compare
| @@ -28,25 +28,11 @@ class Button extends ClickableComponent { | |||
| * @return {Element} | |||
| * The element that gets created. | |||
| */ | |||
| createEl(tag = 'button', props = {}, attributes = {}) { | |||
| createEl(props = {}, attributes = {}) { | |||
There was a problem hiding this comment.
After some thought, I'm unsure whether we want to change the method signature altogether. It totally makes sense to me to ignore it and always create a button element but it feels weird for this method to have a different signature from all other createEls.
There was a problem hiding this comment.
I agree. I'm going to roll that back.
misteroneill
force-pushed
the
buttons-are-buttons
branch
from
04c830f to
aa2c4ef
Compare
misteroneill
force-pushed
the
buttons-are-buttons
branch
from
2535314 to
566d5bc
Compare
Remove deprecation for a Button created with a non-button element - throw instead.
Requirements Checklist