feat: make overlays mutable #223

usmanonazim · 2023-06-08T13:36:30Z

Description

Related Ticket
Currently, overlays cannot be added/removed individually - this change introduces the ability to add/remove overlays individually (customer request)

Specific Changes proposed

New add function for adding overlays
New remove function to remove individual overlays
Updated docs to reflect the above changes

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
Reviewed by Two Core Contributors

usmanonazim · 2023-06-08T13:37:09Z

package.json

@@ -89,7 +89,6 @@
      "git add"
    ],
    "README.md": [
-      "npm run docs:toc",


had to remove this as this errors out whenever a change to the readme is made

usmanonazim · 2023-06-08T13:42:48Z

src/plugin.js

@@ -301,6 +302,7 @@ videojs.registerComponent('Overlay', Overlay);
 * @param    {Object} [options={}]
 */
 const plugin = function(options) {
+  const self = this;


needed to be able to reference this in the add and remove fns, not sure if this is the best way to be able to do that

It's a fine practice, though I would suggest calling it something like player as that's more meaningful. That way, further down the plugin, future developers don't need to wonder what self refers to.

alex-barstow · 2023-06-12T21:24:06Z

README.md

@@ -18,6 +18,11 @@ Maintenance Status: Stable

 - [Getting Started](#getting-started)
 - [Documentation](#documentation)
+  - [API](#api)
+    - [`player.overlay()`](#playeroverlay)
+    - [`overlay.overlays`](#overlayoverlays)


Maybe this should be a getter, ex. getOverlays(), rather than a property?

Good point, will make that change now :)

I would suggest calling it get() to align with the other new methods.

misteroneill · 2023-06-13T14:06:20Z

README.md

@@ -18,6 +18,11 @@ Maintenance Status: Stable

 - [Getting Started](#getting-started)
 - [Documentation](#documentation)
+  - [API](#api)
+    - [`player.overlay()`](#playeroverlay)
+    - [`overlay.overlays`](#overlayoverlays)


I would suggest calling it get() to align with the other new methods.

misteroneill · 2023-06-13T14:08:12Z

src/plugin.js

@@ -301,6 +302,7 @@ videojs.registerComponent('Overlay', Overlay);
 * @param    {Object} [options={}]
 */
 const plugin = function(options) {
+  const self = this;


It's a fine practice, though I would suggest calling it something like player as that's more meaningful. That way, further down the plugin, future developers don't need to wonder what self refers to.

misteroneill · 2023-06-13T14:09:55Z

src/plugin.js

+      item.el().parentNode.removeChild(item.el());
+      self.overlays_.splice(index, 1);
+    } else {
+      throw new Error('overlay is invalid and cannot be removed');


I'm not sure throwing is the right thing here. It may be better to use player.log.warn instead.

Changed the error throwing to be a log.warn 🥳

misteroneill · 2023-06-13T14:10:53Z

src/plugin.js

+   *
+   * @return The array of overlay objects currently used by the plugin
+   */
+  function getOverlays() {


As mentioned above, I think get would be a better name here.

README.md

index.html

misteroneill · 2023-06-15T14:00:45Z

src/plugin.js

+  return {
+    add,
+    remove,
+    get
+  };


Longer-term, we'll want to change this to be a class-based plugin, but this works for now! 👍

usmanonazim added 6 commits June 6, 2023 16:20

implement initial add/remove fns

c55e6f2

add tests for add/remove fns

f557e2d

implement remove functionality with tests

f408eed

update 'add' fn test

8c500a9

update docs to include add/remove fns and remove docs:toc stage

5089448

update index.html example

04aa727

usmanonazim commented Jun 8, 2023

View reviewed changes

update readme toc

f0b3036

usmanonazim commented Jun 8, 2023

View reviewed changes

fix refactored docs

8b05d30

alex-barstow reviewed Jun 12, 2023

View reviewed changes

refactor overlays property to be a getter fn

6584d65

misteroneill reviewed Jun 13, 2023

View reviewed changes

usmanonazim added 2 commits June 13, 2023 16:14

refactor 'getOverlay' to 'get'

0b5d930

update readme toc

b2ca411

usmanonazim requested review from misteroneill and alex-barstow June 14, 2023 14:07

roman-bc-dev reviewed Jun 15, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

roman-bc-dev reviewed Jun 15, 2023

View reviewed changes

index.html Outdated Show resolved Hide resolved

change example usage to use const instead of var

d4a32a2

roman-bc-dev approved these changes Jun 15, 2023

View reviewed changes

usmanonazim merged commit a6c0353 into main Jun 15, 2023

usmanonazim deleted the feat/overlays-mutable branch June 15, 2023 15:34

misteroneill approved these changes Jun 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make overlays mutable #223

feat: make overlays mutable #223

usmanonazim commented Jun 8, 2023 •

edited

Loading

usmanonazim Jun 8, 2023

usmanonazim Jun 8, 2023

misteroneill Jun 13, 2023

alex-barstow Jun 12, 2023

usmanonazim Jun 13, 2023

misteroneill Jun 13, 2023

misteroneill Jun 13, 2023

misteroneill Jun 13, 2023

misteroneill Jun 13, 2023

usmanonazim Jun 13, 2023

misteroneill Jun 13, 2023

misteroneill Jun 15, 2023

feat: make overlays mutable #223

feat: make overlays mutable #223

Conversation

usmanonazim commented Jun 8, 2023 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmanonazim commented Jun 8, 2023 •

edited

Loading