-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Show apps in header #3008
Show apps in header #3008
Conversation
@juliushaertl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @skjnldsv, @Henni and @jancborchardt to be potential reviewers. |
c037462
to
336d376
Compare
What happens when I open Notes in your example? Will there be an indicator that I am using Notes anywhere? |
I like it a lot! |
But this is "Up to 4 apps" - > otherwise the first 3 apps are shown plus the menu opener ;) |
🙈 Changed it. I was not counting the apps management as an app. |
@raimund-schluessler I would prefer to show the icon somwhere, not only an indicator at the 3 dots menu. Does anyone have an idea how we could do that? |
Maybe by showing the current app as the first 🤔 @juliushaertl |
I don't think this are a good solution. It breaks after the third/four app. I talked with @jancborchardt at the Berlin conference that maybe the best solution is to have navigation on the right. https://codepen.io/nikhil/pen/sicrK (example) Why I think its best? Its versatile and works on mobile. I am with a little time to make a proper mockup so I hope that I can explain what I mean. |
I would be fine with that too :) |
@Espina2 We also had some discussion about that over at https://help.nextcloud.com/t/is-this-the-most-awesome-or-what/6163/3 I'll just quote @jancborchardt on this:
|
Oh common guys, i barely have the time to follow everything on github. What can I do if you discuss on nextcloud forums now? 😆 |
336d376
to
5c01c9f
Compare
This is not new, even before you launch the version with icons on the topbar I have called attention to @jancborchardt at conference that in my opinion this is not a good solution. And like you quote
The solution that I am suggesting don't add extra space. You only remove the icon for the second menu entry and add them to the first level. So you have icon + label on the first "menu" and only label on the second, because it's not needed the icon. Tooltips is a crime in User Experience, if you need them its because the information or the interface isn't clear enough. For not talking about mobile that don't have "hover" capabilities. The solution that you are working now, its patching a problem not resolving them. This is my opinion. |
🚨 Call the UX Police 😂 I think @Espina2 proposal could work. I agree that app names are not necessary as long as we have distinct icons. Problem could be external sites. We should add an option to upload a custom icon there. |
I am not at all an UX expert, and I get that a single user feedback and preferences have a really limited usefulness. |
I think they work quite well here, especially for a menu, that you use more often. At the beginning you might need some information about what the icons will do, but after some time you'll start to remember them, so there is no need to show a title always and we can use a tooltip instead.
I would like to keep the current menu for mobile, as requires no screen space while not using it. Just as @SonRiab proposed.
I don't like that very much either, it was just an idea to avoid a too crowded header bar.
Ok, that could work. Feel free to do a mockup. 👍 |
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.
Some code nitpicking inline, and bigger design considerations here:
- Only showing 4 apps until having the »more apps« menu is too few. Let’s increase it to 8 apps. This will also fix the issue @Espina2 mentioned. Of course on mobile or when the screen gets narrower, the menu should show accordingly to make sure there’s no overflow with the username or avatar.
- How about we layout the »more apps« menu like user menu, in a list view. Fits better rather than showing bigger icons in the dropdown than there are in the bar.
- If app from more-menu is open, don’t put it first in list. Either:
- put it as last in the list (before the »more apps« menu icon), switching out the one which was there before temporarily
- or just put a triangle below the 3-dot-menu to indicate it, and don’t switch places
- Fix placement issues of notifications and search on the right
- Consider opening more apps and user menu on hover, like menus on Amazon
- Again, we won’t introduce a left bar for the apps again because we had that in the past and decided against it because the apps/content should get as much space as possible. We have a header already.
core/css/header.scss
Outdated
text-align: center; | ||
color: #1d2d44 !important; | ||
vertical-align: top !important; | ||
position: relative; |
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.
Spaces → tabs :)
core/css/header.scss
Outdated
float: left; | ||
display: inline-block; | ||
vertical-align: top !important; | ||
height:44px; |
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.
Spaces → tabs
core/css/header.scss
Outdated
} | ||
|
||
#appmenu li a { | ||
opacity: 0.6; |
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.
No leading 0 required here :)
core/css/header.scss
Outdated
#appmenu li a { | ||
opacity: 0.6; | ||
margin: 0; | ||
padding: 12px 0px; |
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.
No unit required when it’s a 0-value :)
core/css/header.scss
Outdated
border-top-right-radius: 0; | ||
margin-top: 0; | ||
border: none; | ||
color: #333; |
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.
Should we rather do this with an opacity value, or best do we have a variable for that?
core/css/header.scss
Outdated
-moz-filter: drop-shadow(0 0 5px rgba(150, 150, 150, 0.75)); | ||
-ms-filter: drop-shadow(0 0 5px rgba(150, 150, 150, 0.75)); | ||
-o-filter: drop-shadow(0 0 5px rgba(150, 150, 150, 0.75)); | ||
filter: drop-shadow(0 0 5px rgba(150, 150, 150, 0.75)); |
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.
No leading 0 required for the 0.75 either :)
core/css/header.scss
Outdated
|
||
#appmenu li:hover a:before, | ||
#appmenu li a.active:before { | ||
content: " "; |
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.
Single quotes in CSS
core/css/header.scss
Outdated
width: 0; | ||
position: absolute; | ||
pointer-events: none; | ||
border-bottom-color: white; |
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.
#fff
core/css/header.scss
Outdated
pointer-events: none; | ||
border-bottom-color: white; | ||
border-width: 7px; | ||
top:36px; |
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.
space after colon
core/css/header.scss
Outdated
top:36px; | ||
transform: translateX(-50%); | ||
left: 50%; | ||
z-index: 100; |
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.
On this whole block: spaces → tabs ;)
This is not a valid point, Like I already said this left menu will have the same space. I only see advantages with that approach. First you can navigate directly to a page in a app without open the "home" app page. The navigation are on the same place and are more natural. Like we have now you change from context from two different locations, one on the "header" and another on the left bar. Can you say why this is a bad pattern or give some reason to why you think this will not work? And I want the opinions of all the others designers here |
It won’t. We already have the header bar there with room for icons. The left menu will be an additional element.
How is this different from the approach outlined here? Maybe it’s not entirely clear from the screenshot you posted. Please do a detailed mockup in that case.
Yes, precisely for the reason that they are in the same place. Because the icons will be confused as icons for the other navigation entries, or conflict with navigation entries where there are icons. It will be a mass of icons on the left, some with text, some without. They do not have the same relevance so they don’t need to be in the same place. Anyhow. A bigger issue I have here is that we discussed a lot and still didn’t change anything from the status quo. The direct menu as done by @juliushaertl is the top rated app in the store and really useful to let people know we have a bunch of apps – which is not apparent at the moment. |
I support @jancborchardt, we have to go forward here. The biggest issue with @Espina2 approach is the horizontal space. And the app navigation + the sidebar is not a typical multi level menu, as the sidebar can function quite differently in different apps. The biggest issue with the current approach is the mystery meat. But at least it would not be as mysterious as it is now. We could add some height to the header and add labels beneath the app icons. But I think this should not block us implementing it. |
Like I said more than one time, the horizontal space is the same, it will not add extra space. This have exact the same horizontal space as before. So this issue doesn't exist at all. Can you point wich app use the bar different? Because I can't see any advantage with my approach. @jancborchardt I dont get what you mean with this
At the moment I don't have the time to make an high fidelity mockup so its the best I can do. |
5c01c9f
to
72679af
Compare
@jancborchardt I've rebased and fixed some more issues. I'll try to get this a bit more polished tomorrow. 🙈 The code is still a bit messy. If you want to jump in, feel free to do so. I've updated the todo list in the first post as well. |
71dcb9f
to
a55c2f7
Compare
Ok, this should be ready to review now. I've also updated the screenshots in the first post. cc @jancborchardt @skjnldsv @eppfel @nextcloud/designers |
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
8a6325a
to
b8ef616
Compare
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.
Tested and works in IE11, Firefox, Chrome, Safari 👍
@nextcloud/designers Please review |
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.
Had no problems so far with Safari and Chrome on my instance 💪
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.
Works nicely
This is a experimental implementation of what we've discussed with @jancborchardt @eppfel and @skjnldsv to make apps accessible directly without a popover. See juliusknorr/direct_menu#51
I'm still not totally happy with this, but I guess we should have some discussion so please have a look, @nextcloud/designers
Screenshots (updated)
Up to 8 apps:
More than 8 apps:
Mobile:
ToDo