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

fix(FEC-8448): after 'learn more' is clicked, fullscreen button is not responding #261

Merged
merged 9 commits into from
Aug 6, 2018

Conversation

odedhutzler
Copy link
Contributor

@odedhutzler odedhutzler commented Aug 5, 2018

Description of the Changes

when you click on 'learn more' in IMA (when on full screen) it resize to the regular size. because IMA is an iframe, we cannot listen to fullscreen change event in it's document.

on each render of the full screen component, I am checking if it is different from the previous state , and renders accordingly.

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

@odedhutzler odedhutzler self-assigned this Aug 5, 2018
@@ -42,6 +43,7 @@ class FullscreenControl extends BaseComponent {
*/
constructor(obj: Object) {
super({name: 'Fullscreen', player: obj.player});
this._prevFullscreenState = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

better to take it from the state
this._prevFullscreenState = obj.fullscreen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yairans , @dan-ziv - checking for the status in the componentWillMount . WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

no
should be in componentWillUpdate

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.

@odedhutzler
No need to save local full screen state since its manage in the store.
add the componentWillUpdate() lifecycle hook and just check if nextProps.fullscreen is different from the the current return value of _isFullscreen.
In that case just call fullscreenEnterHandler

odedhutzler and others added 3 commits August 6, 2018 16:41
calling change handler in the component will mount function.
@odedhutzler odedhutzler merged commit 706881f into master Aug 6, 2018
@odedhutzler odedhutzler deleted the FEC-8448 branch August 6, 2018 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants