Restorations and Removals #2406
Conversation
misteroneill
changed the title
misteroneill
changed the title
| * | ||
| * @type {Function} | ||
| */ | ||
| videojs.EventTarget = EventEmitter; |
There was a problem hiding this comment.
if we are changing EventEmitter to EventTarget, I'd prefer to just rename it internally as well and not just on export.
|
Very nice. Thanks for cleaning up the old deprecations. +1 on Gary's comment I think we should also deprecate round and export roundFloat by that name. |
|
Is |
|
Ha, no, probably not. I doubt anyone's using it anyway. Sent from mobile |
|
I vote for not exporting it and considering excising it from the codebase. |
|
Yeah works for me. Sent from mobile |
|
Sounds good. Wasn't sure about renaming |
| if (typeof subClassMethods.init === 'function') { | ||
| log.warn('Constructor logic via init() is deprecated; please use constructor() instead.'); | ||
| subClassMethods.constructor = subClassMethods.init; | ||
| delete subClassMethods.init; |
There was a problem hiding this comment.
I wasn't 100% sure deleteing the init was the best idea here. Had a minor concern that users inspecting the prototype chain could get thrown off, but perhaps the deprecation warning is enough.
There was a problem hiding this comment.
I'm not sure on this one either, but we may want to consider leaving it in case older external code points to the init directly for some reason. Probably better they get the warning than a broken function.
There was a problem hiding this comment.
Good enough reason for me. I'll remove this line.
|
LGTM |
|
BTW, I updated the wiki, too. Apparently you don't have to be an org member to change wiki pages... who knew! |
|
Yep, by default wikis are open to everyone. for better or worse. |
|
At least the wiki is just a git repo, so it's easy enough to roll back. 😄 |
| @@ -329,7 +328,7 @@ export function findElPosition(el) { | |||
|
|
|||
| // Android sometimes returns slightly off decimal values, so need to round | |||
| return { | |||
| left: roundFloat(left), | |||
There was a problem hiding this comment.
I think these should these just be Math.round() now. Back when roundFloat was just round it was used for most rounding.
There was a problem hiding this comment.
Ah, I see. Instead of toFixed with no arguments, use Math.round. Makes sense.
|
LGTM! EventTarget stills feels a little awkward to me exported directly on |
|
We could register |
|
@heff Addressed the issues you raised (except |
|
Great! I think it's good to pull in. Sent from mobile |
misteroneill
force-pushed
the
restorations
branch
from
975a2d4 to
06dbc21
Compare
In case it's referenced by external code, we don't want to `delete` it and potentially cause errors for consumers.
The previous code was added in the transition away from the round-float module to using `Number#toFixed`. It was kind of a round-about and inelegant way of achieving this - casting to string and back to number.
Closes #2405 by restoring several properties/methods for 5.0. Follows on from the discussion in #2402.