Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

MV-301 block horizontal screen on mobile devices #128

Merged
merged 8 commits into from
Mar 1, 2023

Conversation

alexmaar
Copy link
Contributor

No description provided.

Comment on lines 8 to 13
style={{
backgroundImage: "url('/images/videoroomBackground.png')",
backgroundRepeat: "no-repeat",
backgroundSize: "contain",
backgroundPosition: "center",
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good reason to write it as inline style?

if (!window.visualViewport) return;

setIsHorizontalOrientation(
window.visualViewport.width > window.visualViewport.height && window.visualViewport.height < 400
Copy link
Contributor

Choose a reason for hiding this comment

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

There are devices that fail this condition due to height. The Chrome's dev tools provide S20 Ultra with 912x412 viewport that shows broken desktop variant. I own a device with the same viewport and it works, but probably due to menu "consuming" at least 12px.

Playing with "responsive" setting in device toolbar in Chrome, it seems the page starts to be usable at ~650x, maybe that should be the limit here?

const BlockingScreen: React.FC<{ message?: string }> = ({ message }) => {
return (
<div
className="absolute inset-0 z-30 flex h-full w-full flex-col items-center justify-center gap-y-4 bg-brand-sea-blue-100 font-aktivGrotesk text-brand-dark-blue-500"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be a nitpick (i.e. don't waste your time if its not a trivial fix), but when I'm hiding browser's address bar, the viewport or something changes and I'm able to see a part of the page below, here's a video of that:

Screen_Recording_20230217_150402_Chrome.mp4

@@ -137,6 +137,13 @@
background-size: 50%;
background-position: left -30% top 200px, right -30% bottom -15%;
}

.room-page {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name it so it is known this class changes the background? Something like .room-page-bg I feel like this would be more idiomatic with the tailwind style

{isHorizontalMobile && <BlockingScreen message="Turn your screen to resume the call." />}
<div
className={clsx(
"room-page h-[100dvh] w-full",
Copy link
Contributor

Choose a reason for hiding this comment

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

dvh left

<div
className={clsx(
"absolute inset-0 z-30 flex h-full w-full flex-col items-center justify-center gap-y-4 bg-brand-sea-blue-100 font-aktivGrotesk text-brand-dark-blue-500",
"bg-videoroom-background bg-contain bg-center bg-no-repeat"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do room-page need a custom CSS class with background but the blocking screen can have only tailwind classes? Can we unify this?

@alexmaar alexmaar merged commit 731e21b into master Mar 1, 2023
@alexmaar alexmaar deleted the feature/block-horizontal-screen-on-mobile branch March 1, 2023 09:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants