Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Full-screen panels on small screens #2289
Changes from 9 commits
cfdec92
1da97b9
83044f3
f1d8f18
b5b5819
a7d5dbd
26a627d
896686e
98bdf27
5e975d1
382d1de
5986053
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 thehidden
attribute?There was a problem hiding this comment.
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 likehidden
would be more appropriate I could change it.There was a problem hiding this comment.
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. Usinghidden
instead of visibility would change the way it works(visibility: visible
keeps the element taking up layout space, wherehidden
doesdisplay: none
), and I'm not sure if there are assumptions else where that require that. But I would preferhidden
in both cases if there are no issues.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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 usehidden
here but leave<MobilePortraitNav/>
be for now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine 👍🏻