-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix(NcAppNavigation): small screen support #4767
Conversation
@nextcloud-libraries/designers Design review request :) |
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.
wooow 🥳
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.
LGTM from design perspective for now. Improvements should be made later
I forgot to mention, that it is chained on #4633 and only the last one commit is related |
Looks good, I would say we can merge. Mostly the "different color background" case wouldn't happen, and that's the only case where there's maybe we can improve things. We can check the behavior on different apps and see if we need any enhancements for design, so we can merge 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.
Looks good from my side as well @ShGKme! :) Only a detail for follow-up: The toggle does not need an extra square background, it just needs a circular color-main-background as default
I wanted to make visible that this button is connected to the navigation sidebar and remove displaying any background content like the "ex" here: Do you think this is redundant or there is a better way to do this? |
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
0aa20b3
to
68fbe74
Compare
@ShGKme I would say it is redundant. It's just a floating button, the screenshot looks fine as is and in most cases the page is being interacted with and it's clear it's a button. :) |
The only problem with the sidebar button is a lot of users can not find it if it overlays page content (got complains for this quite often (I think because it had no background and the icon mix up with text underneath). |
☑️ Resolves
There are 2 issues with a small screen:
🖼️ Screenshots
Toggle button on a different background:
🚧 Tasks
🏁 Checklist