-
Notifications
You must be signed in to change notification settings - Fork 155
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
Small screen adjustments for mobile PWA usage #2184
base: the-future
Are you sure you want to change the base?
Small screen adjustments for mobile PWA usage #2184
Conversation
@Reinachan here it is :) |
Oh btw there is still more to do here but let's get this merged first before the PR gets any bigger 👍 |
Oh shit i forgot to comment this in the end lol this can be reviewed |
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.
Thank you for the PR! This is generally pretty great! Just some minor things here and there that I've commented on :)
@media (max-width: 452px) { | ||
width: 300px; | ||
} | ||
|
||
max-height: 90vh; |
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.
Your changes result in the searchbar being hidden by the results dropdown while typing on iOS (at least on my phone with the browser interface on the bottom). I'd suggest making this max height use the new dvh
unit and have a fallback to vh
.
max-height: 85vh; /* less likely to interfere with other stuff */
max-height: 90dvh;
@@ -1509,6 +1510,10 @@ | |||
width: 100%; | |||
max-width: 540px; | |||
margin: 0 auto; | |||
|
|||
@media (max-width: 562px) { | |||
margin: 0 12px; |
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.
Why is the margin larger for a full-page post than for the feed? This just feels inconsistent.
margin-left: 8px; | ||
margin-right: 8px; |
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'm not a fan of this change. At least not on smaller phones. It looks good on my OnePlus 6T but on my iPhone X, it takes up too much space. I suggest you add another media query for smaller screens.
@media (max-width: 380px) {
margin-left: 2px;
margin-right: 2px;
}
This also applies to the full-page posts.
padding: 30px 0 0; | ||
|
||
@media (max-width: 608px) { | ||
padding: 30px 12px 0 12px; |
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.
Too much padding. The existing spacing is already good. Fix the Facebook button instead.
@media (max-width: 768px) { | ||
width: 100%; | ||
margin-left: 8px; | ||
margin-right: 8px; | ||
} |
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.
Refer to my comment on the spacing for feeds. Same applies here.
Description
The current app on small screen sizes is not pretty and in some areas, not even usable. For users like myself that would prefer to use the PWA than the mobile app, this is unfortunate. This PR aims to make minor style adjustments where necessary in order to alleviate these issues!
Changes proposed in this pull request that will only be noticeable on small screens:
Changes proposed in this pull request that will always be noticeable (regardless of screen size):
/cc @hummingbird-me/staff
^i will comment this when it is ready for review