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

feat: captions style controls #73

Merged
merged 29 commits into from
Oct 16, 2017
Merged

feat: captions style controls #73

merged 29 commits into from
Oct 16, 2017

Conversation

dvh91
Copy link

@dvh91 dvh91 commented Oct 4, 2017

No description provided.

@dvh91 dvh91 changed the title feat(captions): adding functionality to change caption style feat: captions style controls Oct 4, 2017
@dan-ziv
Copy link
Contributor

dan-ziv commented Oct 4, 2017

@OrenMe
I'm not sure we want to use the TextStyle enums like this (window.KalturaPlayer.Playkit....)
since it binds the UI to our library. Maybe it's better to expose this also on the player instance like we're doing with the Events.
player.TextStyle....

@dvh91 dvh91 requested a review from OrenMe October 8, 2017 13:21
@dan-ziv dan-ziv self-requested a review October 9, 2017 14:06
Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

  1. added the font family to it so - to add it to model and UI.
  2. save UI model state - when choosing settings and saving the style is set on captions, but when opening menu again the style is not accounted in the UI and is set from default state again
  3. add presets as defined - you can see in V2 in CVAA class
  4. change captions style on the fly when settings are set in the CVAA screen
  5. Add font style - it is EdgeStyles in TextStyle
  6. menus always open upwards and sometimes there's just not enough room upwards
  7. Playkit now expose the TextStyle on the instance so you can change window.KalturaPlayer.Playkit.TextStyle to just this.props.player.TextStyle


const colorOptions = [
{
value: window.KalturaPlayer.Playkit.TextStyle.StandardColors.WHITE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be more useful to set the enum with value and name, like

"WHITE": {
   "value": "...",
   "name": "white"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

and then you can just iterate it to create the colorOptions

Copy link
Author

Choose a reason for hiding this comment

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

@OrenMe Yes, it will be more useful for me. Same for fontEdge and opacity.

];

const opacityOptions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the enum value/name

@@ -97,15 +100,29 @@ class CVAAOverlay extends BaseComponent {
* @memberof CVAAOverlay
*/
renderMainState(): React$Element<any> {
const captionsStyleDefault = Object.assign(new window.KalturaPlayer.Playkit.TextStyle(), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

  • The menu selection should show default and two samples and after user selects his settings the menu should have the setting applied on the label

  • The selected setting (default/sample/custom) should have a V icon on it

  • Opacity controls should be range control(scrubber)

  • Style settings options should be camel cased, currently are all UPPERCASE

  • When there are captions enabled on the video and they appear then the demo example caption is shown above the video active caption - this looks bad - need to seat with UX and figure out what is recommended to to

active: this.state.customTextStyle.fontFamily == fontFamily[key]
}));

var fontStyleOptions = Object.keys(edgeStyles).map(key => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

The font edge change is not shown on the selection menu, and after selecting it keeps showing NONE

Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

@dvh91 by the original design when a custom style is chosen the selection menu should change to show a label with style applied on it.
See here: https://projects.invisionapp.com/d/main#/console/10293603/221545959/preview

@dan-ziv
Copy link
Contributor

dan-ziv commented Oct 15, 2017

@dvh91 I think that importing lodash to our bundle will increase his size significantly.
Do u do that only for isEqual? can we implement this method ourselves and expose it via playkit-js utils?

@dvh91
Copy link
Author

dvh91 commented Oct 15, 2017

@dan-ziv I import a single function and not the whole library which means the bundle won't have the whole lodash package.
I do need this specific util function and it won't matter if it will be my own implementation or lodash's

@dan-ziv
Copy link
Contributor

dan-ziv commented Oct 15, 2017

@dvh91
It is, because now this is another dependency for the project

@dan-ziv dan-ziv merged commit a0fb362 into master Oct 16, 2017
@dan-ziv dan-ziv deleted the captions-style branch October 16, 2017 11:14
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.

None yet

3 participants