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

Add an overlay when the app navigation is open #1122

Merged

Conversation

MarcelRobitaille
Copy link
Collaborator

Fixes #1117

What do you think of this solution? The hamburger menu is still kind of hard to see just because it's gray on gray. I made it so clicking the overlay also closes the navigation.

The alternative that I see is to add our own button to open the navigation in the header and our own button to close the navigation in the navigation itself. We would have to hide the current button. We would also have to use the isMobile mixin to see if the button should be present.

@MarcelRobitaille MarcelRobitaille marked this pull request as draft July 27, 2022 08:47
@seyfeb
Copy link
Collaborator

seyfeb commented Jul 27, 2022

I'm not too deep in this, but just an idea: Is it possible to simply override the buttons css? E.g., add a semi-transparent white background to the button in the non-hover/non-active state? Just for demonstration, sth along the lines of:

.app-navigation > button {
  background-color: rgb(255, 255, 0.6);
  border-top-left-radius: 0;
  border-bottom-left-radius: 0;
}

Screenshot 2022-07-27 at 11 05 57

@github-actions
Copy link

github-actions bot commented Jul 27, 2022

Unit Test Results

     27 files       27 suites   6m 33s ⏱️
   476 tests    476 ✔️ 0 💤 0
4 284 runs  4 283 ✔️ 1 💤 0

Results for commit e7451c5.

♻️ This comment has been updated with latest results.

@MarcelRobitaille
Copy link
Collaborator Author

@seyfeb That could work. We'd need some more logic to show the default gray background on hover/focus when not in the overlay mode.

image

I am leaning towards hiding the stock button and using one in the sidebar itself. It seems cleaner to me, and I've already ranted about the stock button, so I'll spare you from that.

@christianlupus
Copy link
Collaborator

I consider the black/gray semi-transparent background a good solution like already suggested in #907 in another context. Apert, I am no friend of building too much blocks on our own duplicating the work done by the Nextcloud core team.

Signed-off-by: Marcel Robitaille <mail@marcelrobitaille.me>
Signed-off-by: Marcel Robitaille <mail@marcelrobitaille.me>
@MarcelRobitaille
Copy link
Collaborator Author

@christianlupus thanks for the feedback

Apert, I am no friend of building too much blocks on our own duplicating the work done by the Nextcloud core team.

I am open to suggestions for how to avoid this, but I don't really see any option. #1117 was opened because of a shortcoming of the core UI. The options I see are work around that or live with it. From what I can tell, #1117 is in some sense intended behaviour for nextcloud-vue. I see the same thing in other apps (e.g. calendar).

@MarcelRobitaille MarcelRobitaille marked this pull request as ready for review October 14, 2022 12:08
Signed-off-by: Christian Wolf <github@christianwolf.email>
Copy link
Collaborator

@christianlupus christianlupus left a comment

Choose a reason for hiding this comment

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

LGTM

@christianlupus christianlupus merged commit 4c91eac into nextcloud:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation button is hard to see
3 participants