Remove tool bar UI background

[takahirox]

Fixes: #5188

The tool bar UI is too tall especially in the landscape mode on mobile devices.

This PR removes the background of the tool bar UI. The viewport becomes vertically wider.

I'm not sure if you like it because I'm not a designer. I hope designers brush up the design later even if we go with this change.

Concepts and thoughts

• Current my main focus is resolving the problem that the viewport is vertically narrow in landscape mode on mobile devices. Allow to hide the bottom 2D UI bar #5188
• I'm not a designer. I hope we can go with this change so far, and then the designers or artists brush up, improve, or clean up it.
• I think Mic and Share icons should be always visible for user privacy. Users should be always aware of whther their mic, camera, or screen is shared. So I don't think of completely hiding the bottom tool bar.
• I don't want to add complexity. So I don't want to add event handlers (eg: Add button to show/hide tool bar.) And I want consistent design/layout across platforms or device orientation for simplicity. If we change them depending on platforms or device orientation, testing cost will be higher or we will easily ignore some cases in testing.
• Preferred design and layout should be very dependent on admins, scenes, users, events, or use cases. (Actually I have gotten the two feedbacks. Some users prefer always visible icons. Others prefer hide/show tray.) I hope admins or owners edit the design and layout in their custom clients if the default design or layout doesn't satisfy them. So I think it's good that the default is good design and simple implementation for them to edit rather than the default is perfect design and complex implementation.

[takahirox]

Ah, wait. I need to fix them...

[takahirox]

Fixed them

[takahirox]

Lnadscape mode before and after. I'm not sure if the new one looks more beautiful, but at least more usable with vertically wider viewport.

[keianhzo]

Other than the style issue that I point out looks good to me and I think it's more usable than what we had.

I think I'd like it a bit better if the labels had more padding around, something like:

But that's more a design decision.

[takahirox]

@keianhzo

Thanks for the review.

I think I'd like it a bit better if the labels had more padding around

I hope someone (especially designers or artists) makes a follow up PR to adjust or improve the design.

[takahirox]

@netpro2k @keianhzo Do you think this is a proper way to set window-full-size canvas?

[keianhzo]

This looks good to me, I think it should work well but just curious, why did you change entries[0].contentRect?

[takahirox]

Because entries[0].contentRect indicates the red line rect area.

Perhaps updating CSS or React (HTML tag placement) can change it to indicate the window area but not sure how to do.

[keianhzo]

Oh ok, thanks for the explanation.

The useResizeViewport hook takes a node and resizes the viewport according to the latest node size. With this change we are always setting the viewport to the window size which works in this case but breaks the hook contract as we are not resizing based on the viewportRef anymore. It works in this case but it might be confusing for people using the hook in the future so I'd rather keeping the hook contract as it was and pass the right viewport reference to it.

Once the useResizeViewportcode has been reverted, you can assign the viewportRef to the room layout div in RoomLayout.js. That div should cover the entire window if I'm right.

return (
    <div
      className={classNames(styles.roomLayout, { [styles.objectFocused]: objectFocused }, className)}
      ref={viewportRef}
      {...rest}
    >
      {sidebar && <div className={classNames(styles.sidebar, sidebarClassName)}>{sidebar}</div>}
      <div className={classNames(styles.modalContainer, styles.viewport)}>{modal}</div>
      {(toolbarLeft || toolbarCenter || toolbarRight) && (
        <Toolbar
          className={classNames(styles.main, styles.toolbar, toolbarClassName)}
          left={toolbarLeft}
          center={toolbarCenter}
          right={toolbarRight}
        />
      )}
      <div className={classNames(styles.main, styles.viewport, { [styles.streaming]: streaming }, viewportClassName)}>
        {viewport}
      </div>
    </div>
  );

Sorry for missing this on my first review.

[takahirox]

Thanks, updated.

[keianhzo]

You also need to remove the viewportRef from the last div, otherwise we can get unexpected behavior.

[takahirox]

Updated, thanks.

[keianhzo]

I think we can get rid of this polyfill?

https://github.com/mozilla/hubs/blob/cfa9fbf0ca8ffdbed36d9095c43b0a1855463635/src/react-components/room/useResizeViewport.js#L2-L3

It seems to be well supported in all browsers since a while ago: https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

[takahirox]

Thanks for the suggestion.

I think we can get rid of this polyfill?

I want to think of it in another PR if needed.