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.

Sign up for GitHub

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

Jump to bottom

Live and Dvr UI #2234

Merged
merged 14 commits into from Aug 21, 2017
Merged

Live and Dvr UI #2234

merged 14 commits into from Aug 21, 2017

Conversation

karimMourra
Copy link
Collaborator

@karimMourra karimMourra commented Aug 15, 2017 •
edited by egreaves

This PR will...

Display the seekable duration of the dvr stream when at live edge, and display the time diff between live and current when not at live edge in dvr mode.

Why is this Pull Request needed?

To resemble the dvr and live mocks provided in the jira ticket

Are there any points in the code the reviewer needs to double check?

n/a

Are there any Pull Requests open in other repos which need to be merged with this?

view tests updated in https://github.com/jwplayer/jwplayer-commercial/pull/3936

Addresses Issue(s):

JW8-96

@jwplayer-robot
Copy link

Automated tests passed!

Cheers! 🥃 👐 🌮

@robwalch
Copy link
Contributor

View test looks OK for DVR, but not for Live:
http://player-pr-test-jenkins.longtailvideo.com/builds/8934/archive/test/view/

screen shot 2017-08-16 at 2 32 24 pm

screen shot 2017-08-16 at 2 32 12 pm

@karimMourra
Copy link
Collaborator Author

test this please

@jwplayer-robot
Copy link

Automated tests passed!

Cheers! 🥃 👐 🌮

@karimMourra
Copy link
Collaborator Author

@robwalch updated view test page. Changes will kick in once jwplayer/jwplayer-commercial#3936 builds
screen shot 2017-08-17 at 1 57 39 pm

@robwalch
Copy link
Contributor

thanks @karimJWP!

egreaves
egreaves reviewed Aug 17, 2017
View reviewed changes
src/css/controls/imports/controlbar.less Outdated
@@ -181,6 +180,9 @@
.jw-svg-icon-live {
display: none;
}

// padding between elapsed time in DVR mode
margin-right: 1em;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a pixel value for the margin. Using em would result in the spacing changing if the font size were ever to change, which is not what we want.

egreaves
egreaves reviewed Aug 17, 2017
View reviewed changes
src/css/controls/imports/icons.less Outdated
@@ -104,5 +104,8 @@
.jw-svg-icon-dvr {
display: none;
}

// adds padding between elapsed time in DVR mode
margin-right: 1em;
Copy link
Contributor

@egreaves egreaves Aug 17, 2017 •
edited

Choose a reason for hiding this comment

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

See above.

@karimMourra
Copy link
Collaborator Author

@egreaves addressed but we will need a ticket to improve the time texts (duration, elapsed, etc) across all player states. they have a max-width, are centered and the amount of text varies; this results in text spilling out of the box when the time is very precise (i.e. 12:00:00), and added space when less precise (i.e 4:00). the box instead should wrap around the text to allow us to control the spacing between text elements.

@egreaves
Copy link
Contributor

@karimJWP we should address that now so the text looks balanced for the following use cases:

  • position / duration text at larger player sizes
  • counting down the position at small player sizes

@jwplayer-robot
Copy link

Caution when merging! Automated tests either failed or had an error. Please confirm stable build before merging. 0386061

Get some ☕

@jwplayer-robot
Copy link

Caution when merging! Automated tests either failed or had an error. Please confirm stable build before merging. 481363e

Get some ☕

@robwalch
Copy link
Contributor

@karimJWP there is a branch 'improvement/live-dvr-ui' in commercial. Since it's merged you can delete it from the remote or rebase it to make sure this PR build succeeds. Top of the logs show that the commercial branch is being switched:

\n==== CHECKOUT JWPLAYER-COMMERCIAL GIT BRANCH MATCHING PR SOURCE-BRANCH ====
Previous HEAD position was 16dd31c... Merge pull request #3932 from jwplayer/feature/flash-commercial
Switched to a new branch 'improvement/live-dvr-ui'
Branch improvement/live-dvr-ui set up to track remote branch improvement/live-dvr-ui from origin.

@karimMourra karimMourra reopened this Aug 18, 2017
@jwplayer-robot
Copy link

Caution when merging! Automated tests either failed or had an error. Please confirm stable build before merging. 68cc1b0

Get some ☕

@jwplayer-robot
Copy link

Caution when merging! Automated tests either failed or had an error. Please confirm stable build before merging. a0dd6cc

Get some ☕

@jwplayer-robot
Copy link

Caution when merging! Automated tests either failed or had an error. Please confirm stable build before merging. c5844c2

Get some ☕

@karimMourra
Copy link
Collaborator Author

test this please

@egreaves egreaves mentioned this pull request Aug 21, 2017
Merged
@jwplayer-robot
Copy link

Automated tests passed!

Cheers! 🥃 👐 🌮

@egreaves egreaves merged commit 136a208 into master Aug 21, 2017
@robwalch robwalch deleted the improvement/live-dvr-ui branch November 23, 2017 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egreaves egreaves egreaves left review comments

@radium-v radium-v Awaiting requested review from radium-v

@aliciahurst aliciahurst Awaiting requested review from aliciahurst

Assignees

@radium-v radium-v

@egreaves egreaves

@johnBartos johnBartos

@aliciahurst aliciahurst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@karimMourra @jwplayer-robot @robwalch @egreaves @radium-v @johnBartos @aliciahurst