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 size of header menus #16057

Merged
merged 3 commits into from
Dec 7, 2019
Merged

Fix size of header menus #16057

merged 3 commits into from
Dec 7, 2019

Conversation

juliushaertl
Copy link
Member

Header menus that are bigger than the screen height will now be scrollable:
image

Fixes nextcloud/external#140

The height is limited here

max-height: $header-menu-entry-height * 7.5; // half entry
but I think we should keep this limit since it avoids having lengthy menu bars.

@juliushaertl juliushaertl added bug design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jun 24, 2019
@juliushaertl juliushaertl added this to the Nextcloud 17 milestone Jun 24, 2019
@@ -114,6 +114,9 @@
/* Use by the apps menu and the settings right menu */
#apps > ul,
&.settings-menu > ul {
max-height: calc(100vh - #{$header-height});
Copy link
Member

Choose a reason for hiding this comment

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

This is an issue on Apple devices as the nav bar is counted in the 100vh. That's why we stopped using it :/

Copy link
Member

Choose a reason for hiding this comment

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

So this needs to be fixed? What’s the proper way to do it?

Copy link
Member

Choose a reason for hiding this comment

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

Use percent. so it depends on the size of the window height. but it's not going to work here as the % would be relative to the header 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
max-height: calc(100vh - #{$header-height});
max-height: calc(100% - #{$header-height});

Copy link
Member

Choose a reason for hiding this comment

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

Does it works Jan?

Copy link
Member Author

Choose a reason for hiding this comment

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

No as the 100% are calculated from the parent element (which is the popover trigger button at 50px height)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea besides the current approach of using at least two times the header height so we have some additional space. Doesn't look to bad to me:
image

I'm open for suggestions, otherwise we should get this in to at least have some improvement for other browsers.

Copy link
Member

Choose a reason for hiding this comment

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

You won't see the issue with vh on emulator or android devices.
iOS devices does not take the bottom navigation bar of the browser out of the 100vh. Meaning 100vh always include the navigation bar and therefor always scrolls because it's bigger.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This still needs more changes, the right header menu (with the link to settings and the user list) is still having the problem.
And the normal app menu has 2 scroll bars now...

@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
@MorrisJobke
Copy link
Member

@juliushaertl What is the status here? We are close to the beta 1. Should this go into 17 or 18?

@nickvergessen
Copy link
Member

Btw this is still a problem with the external sites app: nextcloud/external#140

Would be nice if we can fix this (maybe for 17.0.1) and backport it to 17 and 16?

juliushaertl and others added 3 commits October 4, 2019 20:27
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

I fixed the duplicate scroll bar by using the existing scroll container. Ready to review from my side.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Fixes the issue with the external sites app now

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

🐘

@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 16, 2019
@skjnldsv skjnldsv merged commit 2959487 into master Dec 7, 2019
@skjnldsv skjnldsv deleted the bugfix/noid/header-menu-scroll branch December 7, 2019 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug design Design, UI, UX, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings-Menu overloaded with more than 3 external sites
5 participants