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

Feature/tr 1252 handle stalled event #305

Merged
merged 14 commits into from Jun 4, 2021

Conversation

btamas
Copy link
Contributor

@btamas btamas commented May 31, 2021

Related to https://oat-sa.atlassian.net/browse/TR-1252

  • Listen to video stalled state
  • Show button to make video reloadable by user
  • Tests for stalled behavior

Test:

  • Have an item with media interaction and have another one with media player shared stimulus
  • Play video from CDN and disconnect
  • You should see a message and reload button that reloads the video when you are online again
  • You should not see the message when your internet is on, but it is slow

@btamas btamas marked this pull request as ready for review May 31, 2021 13:26
@btamas btamas requested a review from meefox June 2, 2021 07:15
@marpesia
Copy link
Contributor

marpesia commented Jun 2, 2021

If I block the URL of the media, it returns a general error and goes back to the list of tests. That's correct?
image

@btamas
Copy link
Contributor Author

btamas commented Jun 2, 2021

If I block the URL of the media, it returns a general error and goes back to the list of tests. That's correct?

I think yes, because the item is not loaded correctly, so it is on the item level not on the interaction level.

@marpesia
Copy link
Contributor

marpesia commented Jun 2, 2021

Approved

Copy link
Contributor

@marpesia marpesia left a comment

Choose a reason for hiding this comment

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

Great job! Very detail :)

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic
  • Changelog is updated according to changes (if applicable)
  • Documentation is updated according to changes (if applicable)

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

The code looks good, nice work.

Unfortunately, I was not able to fully test the feature.

I have 2 small comments, but not blocking

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic
  • Changelog is updated according to changes (if applicable)
  • Documentation is updated according to changes (if applicable)

test/mediaplayer/test.js Outdated Show resolved Hide resolved
Comment on lines +102 to +103
font-size: 50px;
line-height: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: no other way that setting it in pixels? How will it present on devices with high DPI or devices with huge resolution?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it should not be a problem we have conversion for this case sure it could be rem / em

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked how the icon was implemented, and it was pixels, so I used pixels to overwrite them.

Copy link
Contributor

@meefox meefox left a comment

Choose a reason for hiding this comment

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

Looking good, please check small notes from JS.
Screenshot 2021-06-02 at 16 27 06

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic
  • Changelog is updated according to changes (if applicable)
  • Documentation is updated according to changes (if applicable)

@btamas btamas merged commit ebc6bb2 into develop Jun 4, 2021
@btamas btamas deleted the feature/TR-1252_handle-stalled-event branch June 4, 2021 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants