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(error-display): update display on consecutive errors #8485

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

amtins
Copy link
Contributor

@amtins amtins commented Nov 5, 2023

Description

When consecutive errors occur, the ErrorDisplay component is not updated with the new error message. This results in an inconsistent state between the player.error and player.errorDisplay.contentEl().textContent.

player.error() player.errorDisplay.content() player.errorDisplay.contentEl().textContent
player.error('Error 1') Error 1 ✔️ Error 1 ✔️ Error 1 ✔️
player.error('Error 2') Error 2 ✔️ Error 2 ✔️ Error 1 ❌

An example of a use case where updating the error message is useful is :

  • user tries to play media 1 but the media doestn't exist
  • user tries to play media 2 but the media is not compatible
Screencast.from.05.11.23.17.18.53.webm

Specific Changes proposed

  • call the close function before each call to the open function.
    • if errorDisplay is not open, the close function does nothing
    • if errorDisplay is open, the close function executes and triggers the close events, then the open function executes and triggers the open events, ensuring that the content is updated.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

When consecutive errors occur, the `ErrorDisplay` component is not updated with the new error message.
This results in an inconsistent state between the `player.error` and `player.errorDisplay.contentEl().textContent`.

|                         | player.error() | player.errorDisplay.content() | player.errorDisplay.contentEl().textContent |
| ----------------------- | -------------- | ----------------------------- | ------------------------------------------- |
| player.error('Error 1') | Error 1 ✔️      | Error 1 ✔️                     | Error 1 ✔️                                   |
| player.error('Error 2') | Error 2 ✔️      | Error 2 ✔️                     | Error 1 ❌                                  |

An example of a use case where updating the error message is useful is :
- user tries to play media 1 but the media doestn't exist
- user tries to play media 2 but the media is not compatible

- call the `close` function before each call to the `open` function.
  - if errorDisplay is not **open**, the `close` function does nothing
  - if errorDisplay is **open**, the `close` function executes and triggers the close events, then the open function executes and triggers the open events, ensuring that the content is updated.
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Merging #8485 (b6a4769) into main (d535e16) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #8485   +/-   ##
=======================================
  Coverage   82.71%   82.71%           
=======================================
  Files         113      113           
  Lines        7589     7591    +2     
  Branches     1826     1826           
=======================================
+ Hits         6277     6279    +2     
  Misses       1312     1312           
Files Coverage Δ
src/js/error-display.js 100.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@mister-ben mister-ben merged commit 7831046 into videojs:main Nov 28, 2023
10 checks passed
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.

2 participants