refactor player.hasStarted() #4680
Conversation
update doc on hasStarted() I feel 'request' are more suitable than hasStarted (variable) because when I do "if (request === undefined)", it mean if you don't have any request, then we just return current status, it a bit more clear
|
Hi @gkatsev , does this style of writing clearer than the previous one. Cause I'm running through the source and see a lot of js write at old style, if this pull request accepted, I might refactor some more ? |
There was a problem hiding this comment.
Thanks @kocoten1992. We do have a lot of code that we just ported over and didn't refactor. Only had a couple of small comments on this, overall it's a good change.
src/js/player.js
Outdated
| * the boolean value of hasStarted_ | ||
| */ | ||
| hasStarted(request) { | ||
| if (request === undefined || request === this.hasStarted_) { |
There was a problem hiding this comment.
A comment explaining that this is used as a getter or if the value isn't changing, we should just return what we have would be good
src/js/player.js
Outdated
| this.removeClass('vjs-has-started'); | ||
| } | ||
|
|
||
| return this.hasStarted_; |
There was a problem hiding this comment.
Our setters generally do not return anything.
There was a problem hiding this comment.
As @gkatsev said, this is my only concern with the PR.
src/js/player.js
Outdated
| this.removeClass('vjs-has-started'); | ||
| } | ||
|
|
||
| return this.hasStarted_; |
There was a problem hiding this comment.
As @gkatsev said, this is my only concern with the PR.
update doc on hasStarted()
I feel 'request' are more suitable than hasStarted (as a variable) because when I do "if (request === undefined)", it mean if you don't have any request, then we just return current status, it a bit more clear
Requirements Checklist