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

Full-screen panels on small screens #2289

Merged
merged 12 commits into from
Nov 30, 2023

Conversation

lemald
Copy link
Member

@lemald lemald commented Nov 28, 2023

Asana ticket: ⚙️ Views or panels should be full screen for < 800px

There were a couple of places that referenced a breakpoint of 480px from the old breakpoint variables, but I updated them to point to the new one because the new breakpoint was actually the relevant one for that context (the thing that determines which breakpoint the CSS should apply at is based on the useScreenSize breakpoints).

I followed the same pattern as with the mobile portrait nav where the nav components are explicitly hidden when a view is opened over them - for one thing the order of the z-indexes isn't right to have the panel go on top of the nav and switching them around causes issues (the drop shadow of some panels bleeds out over the top nav on desktop).

Copy link

Coverage of commit a7d5dbd

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@lemald lemald marked this pull request as ready for review November 28, 2023 20:34
@lemald lemald requested a review from a team as a code owner November 28, 2023 20:34
Copy link

Coverage of commit 26a627d

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link

Coverage of commit 896686e

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link

Coverage of commit 98bdf27

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

<div className="l-nav__nav-bar l-nav__nav-bar--left">
<div
className="l-nav__nav-bar l-nav__nav-bar--left"
style={{ visibility: navVisibilityStyle }}
Copy link
Member

Choose a reason for hiding this comment

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

question: just wondering, why visibility here instead of something like the hidden attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was working off of how <MobilePortraitNav/> is implemented, but if you feel like hidden would be more appropriate I could change it.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I feel like I'd need to know why that was chosen in <MobilePortraitNav/> originally. Using hidden instead of visibility would change the way it works(visibility: visible keeps the element taking up layout space, where hidden does display: none), and I'm not sure if there are assumptions else where that require that. But I would prefer hidden in both cases if there are no issues.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like using `hidden`` would work? #1719 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll play around with it a bit and see if there are any drawbacks to hidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I don't see any drawbacks to hidden. Is it okay if I use hidden here but leave <MobilePortraitNav/> be for now?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine 👍🏻

Copy link

Coverage of commit 382d1de

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link

Coverage of commit 5986053

Summary coverage rate:
  lines......: 94.8% (3013 of 3177 lines)
  functions..: 74.6% (1259 of 1688 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@lemald lemald merged commit 312ac9e into master Nov 30, 2023
8 checks passed
@lemald lemald deleted the lem-full-screen-panels-on-small-screens branch November 30, 2023 19:38
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.

2 participants