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(FEC-11284): dual screen core changes #606

Merged
merged 9 commits into from
Jun 1, 2021
Merged

Conversation

RoyBregman
Copy link
Contributor

@RoyBregman RoyBregman commented May 26, 2021

Description of the Changes

As part of the dual screen plugin development there are several changes that needed to be done in the ui:

  • Fix dynamic icon existence check to support complete removal and adding of an icon again and again
  • Icon now supports other sizes than 36x36px
  • Move playlist and unmute indication to interactive area
  • Make interactive area grow and shrink dynamically according to top / bottom
  • Make top and bottom bars slide in-out
  • Add blur on interactive area when overlay is active
  • Interactive changes its margin according to wether the ui is open
  • Bottom bar has a new state and class added to identify it was broken to two lines
  • Bottom bar padding / margins were fixed with Dana (reduced size from 68px to 60px)
  • Seek bar hover grow is now both up and down so it remains centred vertically
  • Added new PlayerArea "VideoContainer" inside the video area to support side by side mode in dual screen

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

@RoyBregman RoyBregman requested review from yairans, Yuvalke, dan-ziv, OrenMe and a team and removed request for yairans and Yuvalke May 26, 2021 12:14
* @returns {string} - encoded svg url
* @memberof Icon
*/
getSVGUrl = (path: string): string => {
const svg = `<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 1024 1024" width="36" height="36">${path}</svg>`;
getSVGUrl = (path: string, width: ?number = 36, height: ?number = 36): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be
width?: number = 36, height?: number = 36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be w/o "?" actually . Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@OrenMe
Copy link
Contributor

OrenMe commented May 26, 2021

@RoyBregman please only use FEC in ticket titles. @yairans FYI. Cross dependencies should be defined in FEC ticket and tested in core separately for regressions.

@RoyBregman RoyBregman changed the title feat(FEV-864): dual screen core changes feat(FEC-11284): dual screen core changes May 27, 2021
@RoyBregman
Copy link
Contributor Author

@RoyBregman please only use FEC in ticket titles. @yairans FYI. Cross dependencies should be defined in FEC ticket and tested in core separately for regressions.

Done - updated PR title

src/ui-presets/playback.js Outdated Show resolved Hide resolved
src/ui-presets/live.js Outdated Show resolved Hide resolved
src/components/video-player/video-player.js Show resolved Hide resolved
src/components/seekbar/_seekbar.scss Outdated Show resolved Hide resolved
src/components/interactive-area/_interactive-area.scss Outdated Show resolved Hide resolved
src/components/bottom-bar/_bottom-bar.scss Outdated Show resolved Hide resolved
src/components/bottom-bar/_bottom-bar.scss Outdated Show resolved Hide resolved
@OrenMe
Copy link
Contributor

OrenMe commented May 30, 2021

If we change the UI vars in a way that requires updating the CSS guide than please update it

@RoyBregman RoyBregman merged commit e3df209 into master Jun 1, 2021
RoyBregman added a commit that referenced this pull request Jun 2, 2021
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

4 participants