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

Layout Manager Changes #10155

Closed
wants to merge 10 commits into from
Closed

Layout Manager Changes #10155

wants to merge 10 commits into from

Conversation

vitormateusalmeida
Copy link
Contributor

This PR

  • Modify the presentation to auto enable the auto arrange layout when change the presentation aspect ratio.
  • Removes button to enable/disable auto arrange layout from action bar
  • Adds a button to open a debug modal. Can enable/disable the auto arrange layout on this.
    debug-button
  • Removes some unused classes

@vitormateusalmeida vitormateusalmeida marked this pull request as ready for review July 30, 2020 22:24
@TiagoJacobs
Copy link
Member

TiagoJacobs commented Aug 3, 2020

Hello @vitormateusalmeida it would be nice to have an extra configuration that's the ability to enable the debug window using keyboard ( it can be useful when providing support to a user ).

So my suggestion would be to keep the showDebugWindow and add an extra one showDebugWindowByKeyboard .

Also, please add the browser's user agent on this debug window.

@antobinary antobinary added this to the Release 2.3 milestone Aug 7, 2020
window.addEventListener('resize', this.handleResize, false);
document.addEventListener('keyup', (event) => {
const key = event.key.toUpperCase();
if (SHOW_DEBUG_WINDOW_SHORTCUT && event.ctrlKey && event.altKey && key === 'K') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a good idea to have this shortcut follow some of the other shortcuts, using the accesskey attribute for consistency. It should also be visible in the keyboard shortcut modal.

className={styles.debugModal}
onRequestClose={() => mountModal(null)}
hideBorder
contentLabel="Debug"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized

>
<div className={styles.row}>
<div className={styles.col} aria-hidden="true">
<h3 className={styles.title}>Debug</h3>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized

<div className={cx(styles.col, styles.col_4)} aria-hidden="true">
<div className={styles.formElement}>
<div className={styles.label}>
User Agent:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized

type="button"
onClick={() => navigator.clipboard.writeText(window.navigator.userAgent)}
>
Copy
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized

icons={false}
defaultChecked={autoArrangeLayout}
onChange={() => this.autoArrangeToggle()}
ariaLabel="teste"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized, and should be something meaningful.

<div className={styles.col}>
<div className={cx(styles.formElement, styles.pullContentRight)}>
<p className={styles.desc}>
(it will be disabled if you drag or resize the webcams area)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be localized

// icon="application"
customIcon={(
<i className={styles.debugIcon}>
<svg id="Icons" version="1.1" viewBox="0 0 32 32" xmlSpace="preserve" xmlns="http://www.w3.org/2000/svg" xmlnsXlink="http://www.w3.org/1999/xlink">
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, we could probably have @tylercopeland create a debug icon and use the Icon component instead.

@@ -152,25 +152,32 @@ class PanelManager extends Component {

if (userListSize.width !== oldUserListSize.width && userListSize.width !== userlistWidth) {
this.setUserListWidth(userListSize.width);
this.forceUpdate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK we should be trying to avoid the use of this.forceUpdate. Is there another alternative to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I don't think so, I need to do this to force another update after set the width by layout manager.

@@ -34,6 +34,8 @@ public:
duration: 4000
remainingTimeThreshold: 30
remainingTimeAlertThreshold: 1
showDebugWindow: false
showDebugWindowShortcut: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be better in the shortcut section:

  showDebugWindow:
    accesskey: K
    descId: showDebugWindow

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 changed the name of this parameters to make sense.
enableDebugWindowButton: (to show the button to access the debug window)
enableDebugWindowShortcut: (to enable the shortcut to show debug window)

@KDSBrowne
Copy link
Collaborator

KDSBrowne commented Aug 10, 2020

There are 2 other bugs that were also introduced by #10144 .

  • Panels no longer retain their size when re-opened.
  • Long delay when switching between panels (chat to breakoutroom and vise versa).


componentDidMount() {
window.addEventListener('resize', this.handleResize, false);
document.addEventListener('keyup', (event) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to add another event listener. The button on line 191 just needs accessKey={SHOW_DEBUG_WINDOW_ACCESSKEY} added to it as an attribute.

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 added a setting to disable the button, then the button not be render and the accessKey not be work

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, so the idea was to have the shortcut still functional when the button is hidden.

@KDSBrowne
Copy link
Collaborator

Client crashes with blue screen when you try to select the keyboard shortcuts option from the settings dropdown

image

@KDSBrowne
Copy link
Collaborator

KDSBrowne commented Aug 18, 2020

Page reloads cause a problem with the userlist / userlist toggle button.

  • Panel closes itself on reload.
  • Toggle button displays the wrong state (needs 2 clicks to open) after reload.

user-list-toggle-bug

@KDSBrowne
Copy link
Collaborator

KDSBrowne commented Aug 18, 2020

On mobile, If the user opens the user list they are trapped on the view below and can't close the panel to see the presentation.

image

@KDSBrowne
Copy link
Collaborator

The presentation toolbar disappears when exiting full-screen view

presentation-bug-fullscreen

@vitormateusalmeida
Copy link
Contributor Author

Moved to #10390 and #10341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants