feat(jitsipopover): convert to InlineDialog #1804

virtuacoplenny · 2017-07-19T18:14:56Z

Related jitsi-meet-torture changes: jitsi/jitsi-meet-torture#93

Remove JitsiPopover and use InlineDialog instead.
Bring the remote menu icon into react.
Make vertical filmstrip position:fixed so popper (AtlasKit
dependency) sets InlineDialogs and eventually tooltips to
position:fixed.

Future refactoring will likely be needed, such as having the Remote Video Menu Button take in an arbitrary prop/child to display in its InlineDialog, but the current changes were easier to work with for now and should not put future refactoring in a tougher position.

virtuacoplenny · 2017-08-03T18:38:11Z

Force pushed to resolve conflicts.

virtuacoplenny · 2017-08-03T23:32:27Z

css/_vertical_filmstrip_overrides.scss

@@ -8,6 +8,8 @@
        display: flex;
        flex-direction: column-reverse;
        height: 100%;
+        position: fixed;
+        z-index: $tooltipsZ;


popper will set itself as position:fixed if any of its parents are position:fixed. This is necessary to get the InlineDialogs to pop out of the scrolling filmstrip.

Can we add this as a comment in the code. It will help people that aren't that familiar with css to understand the importance of this particular property here.

virtuacoplenny · 2017-08-03T23:34:36Z

css/_vertical_filmstrip_overrides.scss

+    .raisehandindicator,
+    #dominantspeakerindicator {
+        transform: translate3d(0, 0, 0);
+    }


position:fixed inline dialog will position itself relative to a parent that has translate3d. The icons are inside __toolbar and __toptoolbar, so leaving translate3d on them would cause the inline dialogs to look position:absolute. So, move the hardware acceleration closer to the icons that need them so none of the parents have position:fixed.

virtuacoplenny · 2017-08-03T23:35:11Z

css/main.scss

@@ -49,7 +49,6 @@
 @import 'recording';
 @import 'login_menu';
 @import 'popover';
-@import 'jitsi_popover';


All JitsiPopover references should be removed at this point as it has been replaced with InlineDialog.

virtuacoplenny · 2017-08-03T23:36:22Z

modules/UI/videolayout/LocalVideo.js

@@ -24,6 +24,9 @@ function LocalVideo(VideoLayout, emitter) {
        this._buildContextMenu();
    this.isLocal = true;
    this.emitter = emitter;
+    this.statsPopoverLocation = interfaceConfig.VERTICAL_FILMSTRIP
+        ? 'left bottom' : 'top right';


InlineDialog/popper does not ensure it displays itself fully in the viewport. With the local video on the bottom right, it is possible for stats to them display cut off on the bottom when fully expanded. To make up for this, nudge it up (which is "bottom" due to other css tricks in play).

virtuacoplenny · 2017-08-03T23:36:51Z

modules/UI/videolayout/LocalVideo.js

+
+        const ignoreClick = clickedOnDisplayName
+            || clickedOnPopoverTrigger
+            || clickedOnPopover;


Prevent clicks on InlineDialog, which is not a child of the LocalVideo component, from causing focus.

virtuacoplenny · 2017-08-03T23:37:57Z

modules/UI/videolayout/RemoteVideo.js

-    );
+            </I18nextProvider>
+        </Provider>,
+        remoteVideoMenuContainer);


The piece of dom that used to be hovered over to display the remote menu was not in react yet. It was necessary to put that into react for InlineDialog. That piece of dom was moved into RemoteVideoMenuTriggerButton.

virtuacoplenny · 2017-08-03T23:38:39Z

modules/UI/videolayout/RemoteVideo.js

            this.VideoLayout.handleVideoThumbClicked(this.id);
        }

        // On IE we need to populate this handler on video <object>
        // and it does not give event instance as an argument,
        // so we check here for methods.
-        if (event.stopPropagation && event.preventDefault) {
+        if (event.stopPropagation && event.preventDefault && !ignoreClick) {


Same as LocalVideo, InlineDialog is not a child dom element of RemoteVideo so prevent InlineDialog clicks from causing focus.

virtuacoplenny · 2017-08-03T23:41:35Z

react/features/connection-indicator/components/ConnectionIndicator.js

         */
-        showMoreLink: React.PropTypes.bool,
+        statsPopoverPosition: React.PropTypes.string,


enableStatsDisplay is to prevent clicks from causing the stats popover to display in filmstrip only mode. The statsPopoverPosition is necessary for nudging the stats display for local video in vertical filmstrip mode.

The better design here is for ConnectionIndicator to take in children to pass into the InlineDialog for display, so that a parent can better control stats display instead of relying on ConnectionIndicator. However, this design follows existing, pre-react logic and was just easier to implement at this current moment. Future reactification work (that I will do) will revisit this.

virtuacoplenny · 2017-08-03T23:44:29Z

react/features/filmstrip/components/web/BaseIndicator.js

@@ -27,7 +27,7 @@ class BaseIndicator extends Component {
        iconClassName: React.PropTypes.string,

        /**
-         * The front size for the icon.
+         * The font size for the icon.


typo I found while fixing other stuff. Unrelated but trivial so I'm trying to "sneak it in."

virtuacoplenny · 2017-08-03T23:45:59Z

react/features/remote-video-menu/components/MuteButton.js

+        if (onClick) {
+            onClick();
+        }
+    }


MuteButton and KickButton hookup into redux is not necessary but was done to help clean up what needs to get passed into RemoteVideoMenuTriggerButton/RemoteVideoMenu. In general the remote menu buttons should be smart buttons that know how to dispatch actions that affect a participant.

- Remove JitsiPopover and use InlineDialog instead. - Bring the remote menu icon into react. - Make vertical filmstrip position:fixed so popper (AtlasKit dependency) sets InlineDialogs and eventually tooltips to position:fixed.

yanas · 2017-08-09T20:52:01Z

Some general observations and questions:

When clicking on the remote video thumbnail menu, the menu appears, but if I click again on the 3 dot button it doesn't disappear
Question: is there a way to have less indentation/padding inside the inline dialog:

virtuacoplenny · 2017-08-09T21:04:21Z

I can override the AtlasKit styling to reduce the padding.

yanas · 2017-08-09T22:23:11Z

There's some positioning weirdness with the inline dialogs and just want to check what the constraints are there. So I have those different menus:

The first one seems aligned to the trigger vertically, which kind of shifts it from the top of the thumbnail and is very close to the thumbnail to the right.
The second one is centered vertically to the trigger it seems, which is fine. It also has more space to the right, which is also fine. Just mentioning the differences.
The third one appears weirdly compared to the trigger. I remember that there was some constraint there, but can you remind me what exactly?

virtuacoplenny · 2017-08-09T22:44:01Z

For the third one: "I can't get the inline dialog not to be cut off by the bottom of the screen for local stats, so I moved it up."

virtuacoplenny · 2017-08-10T00:13:12Z

react/features/connection-indicator/components/ConnectionIndicator.js

+                content = { this._renderStatisticsTable() }
+                isOpen = { this.state.showStats }
+                onClose = { this._onStatsClose }
+                position = { this.props.statsPopoverPosition }>


@yanas, you can directly play with positioning on this line. Maybe you want to try another position other than on the left? Maybe bottom/top? You might encounter z-indexing issues but those should be fixable. What you seem to be expecting is the old jitsipopover behavior that uses jquery to nudge the position to where you want and nudge the view to make it fit in the viewport. However, I can't get that behavior with AtlasKit but maybe you can figure out a way and I'm overlooking it. https://atlaskit.atlassian.com/components/inline-dialog

…tays at top of toolbar

virtuacoplenny requested a review from yanas July 19, 2017 18:14

virtuacoplenny assigned yanas Jul 19, 2017

virtuacoplenny commented Aug 3, 2017

View reviewed changes

virtuacoplenny added 3 commits August 3, 2017 16:46

ref(remote-menu): hook KickButton to redux

3cbebe5

ref(remote-menu): hook MuteButton to redux

56e841a

virtuacoplenny mentioned this pull request Aug 4, 2017

feat(tooltips): Use AtlasKit #1794

Merged

modify padding, toggle dialogs

1cda690

virtuacoplenny commented Aug 10, 2017

View reviewed changes

virtuacoplenny added 3 commits August 10, 2017 17:29

pixel push margins to align dialogs, adjust padding of dialogs

4cdc654

add comment about margin for dialog, add file I forgot

b114761

modify indicator markup so the icon can be moved down while trigger s…

00cabd0

…tays at top of toolbar

yanas merged commit 725d39d into jitsi:master Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jitsipopover): convert to InlineDialog #1804

feat(jitsipopover): convert to InlineDialog #1804

virtuacoplenny commented Jul 19, 2017 •

edited

Loading

virtuacoplenny commented Aug 3, 2017

virtuacoplenny Aug 3, 2017

yanas Aug 9, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

virtuacoplenny Aug 3, 2017

yanas commented Aug 9, 2017

virtuacoplenny commented Aug 9, 2017

yanas commented Aug 9, 2017

virtuacoplenny commented Aug 9, 2017

virtuacoplenny Aug 10, 2017

feat(jitsipopover): convert to InlineDialog #1804

feat(jitsipopover): convert to InlineDialog #1804

Conversation

virtuacoplenny commented Jul 19, 2017 • edited Loading

virtuacoplenny commented Aug 3, 2017

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

yanas commented Aug 9, 2017

virtuacoplenny commented Aug 9, 2017

yanas commented Aug 9, 2017

virtuacoplenny commented Aug 9, 2017

Choose a reason for hiding this comment

virtuacoplenny commented Jul 19, 2017 •

edited

Loading