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

patch: initialization of textStyle object #127

Merged
merged 3 commits into from
Oct 9, 2017
Merged

Conversation

dvh91
Copy link

@dvh91 dvh91 commented Oct 4, 2017

Description of the Changes

textStyle object initialized so the UI could understand what the initial textStyle is and show accordingly the captions style options.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@dvh91 dvh91 requested a review from OrenMe October 4, 2017 14:57
Copy link
Contributor

@dan-ziv dan-ziv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move initialization to constructor

@OrenMe
Copy link
Contributor

OrenMe commented Oct 8, 2017

@dvh91 don't know if you deleted the default Pull Request template or it didn't appear but we have it there for a reason so please add it to the first comment of this PR(just edit it) and add details and tick the corresponding check boxes after you save it.
Thanks!

@OrenMe
Copy link
Contributor

OrenMe commented Oct 8, 2017

@dan-ziv ES6 classes member initialization can be done like this in general, and it is exactly the same as constructor initialization AFAIK.
Only issue is the fact we do some in constructor and some in member definition, so there's no alignment.
By the way, if you check the actual transpiled class you will see it does the _textTrack init in the class function definition, e.g.

function Player() {
   ...
   this._textTrack = new ...
   ...
}

@dvh91
Copy link
Author

dvh91 commented Oct 8, 2017

@OrenMe - Updated the first comment with the pull request template.

@dan-ziv
Copy link
Contributor

dan-ziv commented Oct 8, 2017

@OrenMe
I know that,
The reason I asked this is only for consistency.

@dan-ziv dan-ziv closed this Oct 9, 2017
@dan-ziv dan-ziv reopened this Oct 9, 2017
@dan-ziv dan-ziv merged commit 83bf106 into master Oct 9, 2017
@dan-ziv dan-ziv deleted the textstyle-initialization branch October 9, 2017 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants