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

Add time display to viewer controls #225

Merged
merged 1 commit into from
May 29, 2020
Merged

Conversation

mmattel
Copy link
Contributor

@mmattel mmattel commented Apr 4, 2020

Based on the work of @tomschulze on original PR #223 and #224 (both closed).
Fixes #150 (suggestion from @hodyroff)
Tested on my dev environment and works as described

This adds the time to the video controls, pls see screenshot:

image

Original comment from @tomschulze:

A few remarks:

  • I wasn't sure about your styling and I didnt wanna mess with your styles, so I used the viewer__control__count css class to style the display for test purposes (which disappears on small screens)
  • totalDisplayTime is computed because it doesn't change during the video
  • I put getDisplayTime() in helper.js since it seemed more like a utility function to me
  • the function showTimeInfo() aggregates the displayed string

@mmattel mmattel added enhancement New feature or request 3 - To review labels Apr 4, 2020
@mmattel mmattel requested a review from davitol April 6, 2020 10:12
@phil-davis phil-davis removed their request for review April 7, 2020 10:55
@phil-davis
Copy link
Contributor

The code looks believable, but I am not the one to review JS.

src/scripts/helper.js Outdated Show resolved Hide resolved
src/scripts/helper.js Outdated Show resolved Hide resolved
src/scripts/helper.js Outdated Show resolved Hide resolved
@mmattel mmattel force-pushed the add_time_to_videos branch from 8413d2e to 2684e45 Compare April 8, 2020 12:58
@mmattel
Copy link
Contributor Author

mmattel commented Apr 8, 2020

@DeepDiver1975 fixed all your comments 😃

@phil-davis phil-davis requested a review from DeepDiver1975 April 8, 2020 13:22
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

TypeError: moment.duration(...).format is not a function
    at s.getDisplayTime (eval at globalEval (jquery.js:328), <anonymous>:13:215107)
    at s.totalDisplayTime (eval at globalEval (jquery.js:328), <anonymous>:13:53036)
    at mi.get (eval at globalEval (jquery.js:328), <anonymous>:7:26688)
    at mi.evaluate (eval at globalEval (jquery.js:328), <anonymous>:7:27838)
    at s.eval [as totalDisplayTime] (eval at globalEval (jquery.js:328), <anonymous>:7:29697)

@mmattel
Copy link
Contributor Author

mmattel commented Apr 9, 2020

@DeepDiver1975 you are challenging me 😄
please take a look on the update, thanks

@DeepDiver1975
Copy link
Member

24 / 30 - nobody will understand this ....

Screenshot from 2020-04-14 10-42-39

@mmattel
Copy link
Contributor Author

mmattel commented Apr 14, 2020

Hello @DeepDiver1975
First of all, thanks for reviewing and your patience.
When the video plays, you see the left part of the number block counting, while the right part shows the total time. Depending on the length, you see ss / ss or mm:ss / mm:ss or hh:mm:ss / hh:mm:ss. What is your suggestion of presentation?

@DeepDiver1975
Copy link
Member

What is your suggestion of presentation?

At least 00:24 / 00:30 I'd say.

@mmattel
Copy link
Contributor Author

mmattel commented Apr 14, 2020

OK, will try to have fixed minutes and optional hours as suggested

@mmattel mmattel force-pushed the add_time_to_videos branch 2 times, most recently from b2fe6e9 to eb185ec Compare April 15, 2020 13:18
@mmattel
Copy link
Contributor Author

mmattel commented Apr 15, 2020

Hello @DeepDiver1975 implemented your suggestion

@mmattel
Copy link
Contributor Author

mmattel commented Apr 20, 2020

@DeepDiver1975 may I ask for a review - would like to get this finalized 😃

@mmattel mmattel force-pushed the add_time_to_videos branch from eb185ec to 8eec13b Compare April 29, 2020 08:27
@mmattel
Copy link
Contributor Author

mmattel commented Apr 29, 2020

Just seen that recently upgraded easytext from another PR we had a conflict in packages.json to be solved - done.

@deepdiver please review so we can merge it - many thanks 😊

@DeepDiver1975 DeepDiver1975 merged commit e73a314 into master May 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the add_time_to_videos branch May 29, 2020 12:49
@mmattel mmattel mentioned this pull request May 29, 2020
31 tasks
@HanaGemela HanaGemela added this to the QA milestone Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - To review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add time to videos
4 participants