[Player-54] refacto modal position #124

jparez · 2025-02-10T14:18:46Z

Description

Add an option to display the toolbar on the left or right side of the screen
Calculate the position of the modal when opening for the first time
Make the widget modal draggable inside the video wrapper
Add a magnetic feature when the modal is dragged near its starting position
Small refactor and fixes (icons, color theme, etc.)

Demo

PLAYER-54-modal-draggable.mp4

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

…ace by this.instance.video.srcObject

…handle in css

… for determining toOpen from dispatch to reducer in OVERLAY_OPEN action

README.md

src/DeviceRendererFactory.js

src/plugins/util/OverlayPlugin.js

caarmen · 2025-02-20T14:39:16Z

src/plugins/util/OverlayPlugin.js

+
+    /**
+     * Creates modal header element with title and close button
+     * @param {?string} title - Header title content


What's preferable ? ?string or string|null?

string|null is clearer

caarmen · 2025-02-20T14:41:05Z

src/plugins/util/OverlayPlugin.js

+     * @returns {HTMLElement} Configured header element
+     */
+    createModalHeader(title) {
+        const header = document.createElement('div');


Why not use a semantic element like header? Is it out of scope for this PR?

Like you said, it's a semantic element used for SEO purposes. The
tag can help improve code understanding for a main header, but I'm not sure it's useful for the readability of a modal. Personally, I prefer using
elements.

It's for SEO but also accessibility (used by screen readers for the visually impaired).

Right, but the app isn't ARIA-compliant, so this alone isn't really relevant.

caarmen · 2025-02-20T14:43:24Z

src/plugins/util/OverlayPlugin.js

+     * @returns {HTMLElement} Configured modal element
+     */
+    createModalElement(classes, width, height) {
+        const modal = document.createElement('div');


<dialog> wouldn't be relevant?

It could be, since the element comes with a built-in API for opening and closing. However, given the legacy code, I'm not sure it's worth the effort to refactor. For a new project, though, it would definitely be a better choice.

caarmen · 2025-02-20T14:45:38Z

src/plugins/util/OverlayPlugin.js

+    }
+
+    /**
+     * Initializes drag-and-drop and positioning for the modal


Is this "drag and drop"? Or just dragging (positioning/moving)?

Is there an action done upon dropping?

It's just dragging, but in my confused mind, 'drag' always comes with 'drop'! >_<

src/plugins/util/OverlayPlugin.js

src/scss/base/_genymotion.scss

pgivel · 2025-02-20T16:45:50Z

src/DeviceRenderer.js

+            this.video.addEventListener(
+                'loadedmetadata',
+                () => {
+                    this.videoWrapper.style.aspectRatio = this.video.videoWidth + '/' + this.video.videoHeight;
+                },
+                {once: true},
+            );


We should check that this doesn't break anything in SiB cc @dpilarsk

pgivel · 2025-02-20T17:09:38Z

src/plugins/KeyboardEvents.js

                ];
            }
            this.instance.root.focus();
            this.keyboardCallbacks.forEach((item, index, array) => {
-                array[index].removeListener = this.instance.addListener(this.instance.root, item.event, item.handler);
+                array[index].removeListener = this.instance.addListener(this.instance.video, item.event, item.handler);


I fear this may break everything on Shadow In Browser? cc @dpilarsk

@jparez After discussing a bit with Dimitri, there is no issue with breaking SiB compatibility, we just have to list the breaking changes in the release changelog. (I think changing the html element that listens for the events is probably a breaking change)

pgivel · 2025-02-20T17:19:25Z

src/store/index.js

+                if (state.overlay.widgetsOpened.length === 0) {
+                    state.overlay.isOpen = false;
+                }
+                if (overlayID) {
+                    state.overlay.widgetsOpened = state.overlay.widgetsOpened.filter((id) => id !== overlayID);
+                } else {
+                    state.overlay.widgetsOpened = [];
+                }


Suggested change

if (state.overlay.widgetsOpened.length === 0) {

state.overlay.isOpen = false;

}

if (overlayID) {

state.overlay.widgetsOpened = state.overlay.widgetsOpened.filter((id) => id !== overlayID);

} else {

state.overlay.widgetsOpened = [];

}

if (overlayID) {

state.overlay.widgetsOpened = state.overlay.widgetsOpened.filter((id) => id !== overlayID);

} else {

state.overlay.widgetsOpened = [];

}

if (state.overlay.widgetsOpened.length === 0) {

state.overlay.isOpen = false;

}

Shouldn't the widgetsOpened be cleared before we check if it's empty?

pgivel · 2025-02-20T17:24:33Z

src/plugins/IOThrottling.js

@@ -191,7 +186,7 @@ module.exports = class IOThrottling extends OverlayPlugin {
        this.container.appendChild(clearCacheDiv);

        // Render into document
-        this.instance.root.appendChild(this.widget);
+        this.instance.root.appendChild(modal);


I don't understand why for some widgets you kept this line while removing it for others but honestly I trust you for it.

for IoThrottling is a mistake :$

pgivel · 2025-02-20T17:25:31Z

src/plugins/MouseEvents.js

@@ -199,7 +199,7 @@ module.exports = class MouseEvents {

            this.mouseCallbacks.forEach((item, index, array) => {
                array[index].removeListener = this.instance.addListener(
-                    this.instance.videoWrapper,
+                    this.instance.video,


(ShadowInBrowser overrides this function and doesn't call the super function, so no worries here)

src/plugins/util/OverlayPlugin.js

pgivel · 2025-02-21T15:39:04Z

src/plugins/util/OverlayPlugin.js

+        const modalWidth = this.widget.offsetWidth || 200;
+        const modalHeight = this.widget.offsetHeight || 100;


Maybe these values could be extracted to a CONST at the begining of this file. Similar comment for the +/- 10 values in the next lines of this function

jparez force-pushed the dev/player-54-refacto-modal-position branch 3 times, most recently from 439b5ad to 189adeb Compare February 10, 2025 16:26

jparez requested a review from pgivel February 10, 2025 16:38

jparez force-pushed the dev/player-54-refacto-modal-position branch from 189adeb to 67eacec Compare February 11, 2025 07:59

jparez force-pushed the dev/player-42-customize-toolbar-order branch 2 times, most recently from eebd8f3 to 7d02725 Compare February 11, 2025 08:26

Base automatically changed from dev/player-42-customize-toolbar-order to main February 11, 2025 08:27

jparez added 10 commits February 11, 2025 09:27

replace code to create modal in plugin by calling createTemplateModal

3fe6799

delete this.stream reference, used only on screencast recording, repl…

ef398f9

…ace by this.instance.video.srcObject

Adding phone button to mimic real phone, refacto the way of video is …

34d2bad

…handle in css

Change name overlay.widgetOpened=> overlay.widgetsOpened + Move logic…

7ab679a

… for determining toOpen from dispatch to reducer in OVERLAY_OPEN action

change CSS theme vars name and value, in accordance to figma

64f0d3a

update keympaaping icon

f9b4509

tempory fix upload widget with new modal system

6dde92b

fix theme color for readonl:y textinput

5a9435e

delete text "status" from ioThrottling widget, in accordance to figma

8d06387

widget draggable and toolbar position

3b43091

jparez force-pushed the dev/player-54-refacto-modal-position branch from 67eacec to 3b43091 Compare February 11, 2025 08:28

caarmen reviewed Feb 20, 2025

View reviewed changes

fixup! widget draggable and toolbar position

c0e5d56

pgivel requested changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Player-54] refacto modal position #124

[Player-54] refacto modal position #124

jparez commented Feb 10, 2025 •

edited

Loading

caarmen Feb 20, 2025

jparez Feb 21, 2025

caarmen Feb 20, 2025

jparez Feb 20, 2025

caarmen Feb 20, 2025 •

edited

Loading

jparez Feb 21, 2025

caarmen Feb 20, 2025

jparez Feb 21, 2025

caarmen Feb 20, 2025

jparez Feb 21, 2025

pgivel Feb 20, 2025

pgivel Feb 20, 2025

pgivel Feb 24, 2025

pgivel Feb 20, 2025

pgivel Feb 20, 2025

jparez Feb 21, 2025

pgivel Feb 20, 2025

pgivel Feb 21, 2025

		const modalWidth = this.widget.offsetWidth \|\| 200;
		const modalHeight = this.widget.offsetHeight \|\| 100;

[Player-54] refacto modal position #124

Are you sure you want to change the base?

[Player-54] refacto modal position #124

Conversation

jparez commented Feb 10, 2025 • edited Loading

Description

Type of change

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

caarmen Feb 20, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jparez commented Feb 10, 2025 •

edited

Loading

caarmen Feb 20, 2025 •

edited

Loading