fix(FEC-8032): when switching to iOS fullscreen in portrait captions displayed in the middle for few seconds #223

odedhutzler · 2018-04-18T11:20:29Z

when detecting iOS and full screen, removing the transition -60px because there is no bottom menu bar.
added environment classes to the player main class to indicate browser and device.
added helper 'playkit' global var to the webpack build.

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

fired an iOSFullscreen action when going to full screen on ios devices. added a class if iosFullscreen state changes respectivly, removed the transition -60px because there is no bottom menu bar

dan-ziv · 2018-04-22T08:24:30Z

@odedhutzler I think this solution is too specific.
You don't really need to save iOSFullscreen state in the store since you already know when u r
on fullscreen and what is your platform.
If the solution is only to set a custom css on iOS we can handle it simply in the render function of the fullscreen component

OrenMe · 2018-04-22T11:49:09Z

src/components/shell/shell.js

@@ -276,6 +277,7 @@ class Shell extends BaseComponent {
    if (this.props.metadataLoaded) playerClasses.push(style['state-' + this.props.currentState]);
    if (this.props.seekbarDraggingActive) playerClasses.push(style.hover);
    if (this.props.playerClientRect && this.props.playerClientRect.width <= 480) playerClasses.push(style.sizeSm);
+    if (this.props.fullscreen && this.player.env.os.name === 'iOS') playerClasses.push(style.iOSFullscreen);


Let's move this condition from between the size condition (row above and below).
Also, let's just add fullscreen state class, e.g. if (this.props.fullscreen) playerClass.push(style.inFullscreen) (or maybe just fullscreen)
and add user agent class, so:

let playerClasses = [style.player, style.skinDefault, this.player.env.os.name, this.player.env.browser.name]; playerClasses.push(props.playerClasses);

Probably need to add the playkit prefix to these new classes.

ref in V2: https://github.com/kaltura/mwEmbed/blob/ba045a91abdee7d0761ba543f5068b4335c04c9f/modules/EmbedPlayer/resources/mw.PlayerLayoutBuilder.js#L152-L177

changing the class selection accordingly

OrenMe · 2018-04-22T14:34:14Z

src/components/shell/shell.js

@@ -265,7 +266,9 @@ class Shell extends BaseComponent {
   * @memberof Shell
   */
  render(props: any): React$Element<any> {
-    let playerClasses = [style.player, style.skinDefault];
+    let playerClasses = [style.player, style.skinDefault,
+      `playkit-${this.player.env.os.name.replace(/ /g, "_")}`,


We can add the prefix as global in webpack.config and add this as token here.
See here how we use it with webpack.DefinePlugin
We define it here:

playkit-js-ui/webpack.config.js

Lines 9 to 13 in afe561f

new webpack.DefinePlugin({

__VERSION__: JSON.stringify(packageData.version),

__NAME__: JSON.stringify(packageData.name),

'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || 'development')

})

and use it here:

playkit-js-ui/src/index.js

Lines 23 to 24 in afe561f

export {UIManager};

export {__VERSION__ as VERSION, __NAME__ as NAME};

so you can add another define in webpack.config ➡️ const CSS_MODULE_PREFIX = 'playkit' and use it in webpack config in the define plugin and also in the style loader config where we define the localIdentName

dan-ziv · 2018-04-23T07:19:07Z

webpack.config.js

@@ -4,11 +4,13 @@ const webpack = require("webpack");
 const path = require("path");
 const PROD = (process.env.NODE_ENV === 'production');
 const packageData = require("./package.json");
+const CSS_MODULE_PREFIX = "playkit-";


Remove the - since this is not part of the prefix

done + beautify

dan-ziv · 2018-04-23T07:19:42Z

src/components/shell/shell.js

@@ -265,7 +267,9 @@ class Shell extends BaseComponent {
   * @memberof Shell
   */
  render(props: any): React$Element<any> {
-    let playerClasses = [style.player, style.skinDefault];
+    let playerClasses = [style.player, style.skinDefault,
+      __CSS_MODULE_PREFIX__+this.player.env.os.name.replace(/ /g, "_"),


Format code!

dan-ziv · 2018-04-23T07:20:16Z

webpack.config.js

@@ -66,7 +68,7 @@ module.exports = {
          options: {
            camelCase: true,
            modules: true,
-            localIdentName: 'playkit-[local]'
+            localIdentName: CSS_MODULE_PREFIX+'[local]'


Format code!

OrenMe · 2018-04-24T18:30:55Z

@dan-ziv please re-review following fixes

dan-ziv · 2018-04-25T07:17:35Z

src/components/shell/shell.js

@@ -7,6 +7,7 @@ import {bindActions} from '../../utils/bind-actions';
 import {actions} from '../../reducers/shell';
 import {KeyMap} from "../../utils/key-map";

+declare var __CSS_MODULE_PREFIX__: string;


Move this declaration to the ui manager

dan-ziv · 2018-04-25T07:22:07Z

src/components/shell/shell.js

-    let playerClasses = [style.player, style.skinDefault];
+    let playerClasses = [style.player, style.skinDefault,
+      __CSS_MODULE_PREFIX__ + '-' + this.player.env.os.name.replace(/ /g, "_"),
+      __CSS_MODULE_PREFIX__ + '-' + this.player.env.browser.name.replace(/ /g, "_")];


No need to re calculate it on every time render has been called. We can calculate it one time in the constructor and use it here.

this._osClass = `${__CSS_MODULE_PREFIX__}-${this.player.env.os.name.replace(/ /g, '-')}`; this._browserClass = `${__CSS_MODULE_PREFIX__}-${this.player.env.browser.name.replace(/ /g, '-')}`;

One more thing is to change the replacer string to '-' instead of '_' since this is our convention in css class names.

dan-ziv · 2018-04-25T08:25:28Z

@odedhutzler update description

…nstructor

…nto FEC-8032

fixed the captions in native ios devices

be5c99f

fired an iOSFullscreen action when going to full screen on ios devices. added a class if iosFullscreen state changes respectivly, removed the transition -60px because there is no bottom menu bar

odedhutzler self-assigned this Apr 18, 2018

odedhutzler requested review from OrenMe, yairans and dan-ziv April 18, 2018 11:20

used fullscreen & env instead of creating new iosfullscreen state

8a47d30

OrenMe requested changes Apr 22, 2018

View reviewed changes

adding os + browser to the player element

7ae02e0

changing the class selection accordingly

OrenMe requested changes Apr 22, 2018

View reviewed changes

using webpack gloabal const for 'playkit' class prefix

5ce7720

OrenMe previously approved these changes Apr 23, 2018

View reviewed changes

Merge branch 'master' into FEC-8032

d5464fc

dan-ziv suggested changes Apr 23, 2018

View reviewed changes

beautify

52c2af3

odedhutzler dismissed OrenMe’s stale review via 52c2af3 April 23, 2018 07:59

dan-ziv suggested changes Apr 25, 2018

View reviewed changes

Dan Ziv and others added 5 commits April 25, 2018 11:25

Merge branch 'master' into FEC-8032

758c5b0

moving to global flow types + creation of env classes in the shell co…

538457c

…nstructor

Merge branch 'FEC-8032' of https://github.com/kaltura/playkit-js-ui i…

39a57f5

…nto FEC-8032

double to single-quots

fa40a65

es6 strings

e7528e2

dan-ziv previously approved these changes Apr 25, 2018

View reviewed changes

fix build

13cf2ad

odedhutzler dismissed dan-ziv’s stale review via 13cf2ad May 6, 2018 08:18

Merge branch 'master' into FEC-8032

4be8a4b

dan-ziv approved these changes May 6, 2018

View reviewed changes

Merge branch 'master' into FEC-8032

b569c4c

odedhutzler merged commit 4da5170 into master May 6, 2018

odedhutzler deleted the FEC-8032 branch May 6, 2018 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(FEC-8032): when switching to iOS fullscreen in portrait captions displayed in the middle for few seconds #223

fix(FEC-8032): when switching to iOS fullscreen in portrait captions displayed in the middle for few seconds #223

odedhutzler commented Apr 18, 2018 •

edited

dan-ziv commented Apr 22, 2018

OrenMe Apr 22, 2018

OrenMe Apr 22, 2018

dan-ziv Apr 23, 2018

odedhutzler Apr 23, 2018

dan-ziv Apr 23, 2018

dan-ziv Apr 23, 2018

OrenMe commented Apr 24, 2018

dan-ziv Apr 25, 2018

dan-ziv Apr 25, 2018

odedhutzler Apr 25, 2018

dan-ziv commented Apr 25, 2018

	new webpack.DefinePlugin({
	__VERSION__: JSON.stringify(packageData.version),
	__NAME__: JSON.stringify(packageData.name),
	'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV \|\| 'development')
	})

	export {UIManager};
	export {__VERSION__ as VERSION, __NAME__ as NAME};

fix(FEC-8032): when switching to iOS fullscreen in portrait captions displayed in the middle for few seconds #223

fix(FEC-8032): when switching to iOS fullscreen in portrait captions displayed in the middle for few seconds #223

Conversation

odedhutzler commented Apr 18, 2018 • edited

CheckLists

dan-ziv commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrenMe commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv commented Apr 25, 2018

odedhutzler commented Apr 18, 2018 •

edited