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

fix(NcAppNavigation): small screen support #4767

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Nov 7, 2023

☑️ Resolves

There are 2 issues with a small screen:

  1. On a very small screen (320px) the 300px navigation is too wide, the toggle button is out of viewport
  2. The toggle button doesn't look good on top of content

🖼️ Screenshots

🏚️ Before 🏡 After
before-2 after-2

Toggle button on a different background:

image

image

image

🚧 Tasks

  • Add max width to make navigation smaller on a small screen
  • Add background to the toggle button

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 7, 2023

@nextcloud-libraries/designers Design review request :)

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

wooow 🥳

Copy link
Contributor

@szaimen szaimen left a 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

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 7, 2023

I forgot to mention, that it is chained on #4633 and only the last one commit is related

@nimishavijay
Copy link

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 :)

Copy link
Contributor

@jancborchardt jancborchardt left a 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

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 8, 2023

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:

image

Do you think this is redundant or there is a better way to do this?

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/app-navigation--small-screen branch from 0aa20b3 to 68fbe74 Compare November 8, 2023 11:05
@ShGKme ShGKme changed the title [WIP] fix(NcAppNavifation): fix on a small screen fix(NcAppNavifation): small screen support Nov 8, 2023
@ShGKme ShGKme added bug Something isn't working 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: app-navigation Related to the app-navigation component labels Nov 8, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Nov 8, 2023
@ShGKme ShGKme marked this pull request as ready for review November 8, 2023 11:10
@susnux susnux merged commit 99edc27 into master Nov 8, 2023
16 checks passed
@susnux susnux deleted the fix/app-navigation--small-screen branch November 8, 2023 11:21
@jancborchardt
Copy link
Contributor

@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. :)

@susnux
Copy link
Contributor

susnux commented Nov 8, 2023

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).

@ShGKme ShGKme changed the title fix(NcAppNavifation): small screen support fix(NcAppNavigation): small screen support Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV] Fix left sidebar from a11y perspective
7 participants