Properly align appnavigationtoggle

[marcoambrosini]

...Upstreaming more talk v-deeps

Before:

After:

Signed-off-by: Marco Ambrosini marcoambrosini@pm.me

[raimund-schluessler]

Honestly, I do not think this is a good idea. The AppNavigationToggle has no own space in our layout, i.e. the app-content does not reserve space for it. This means we have a lot of problems with the AppNavigationToggle as it is positioned already, since it overlaps the content see https://github.com/nextcloud/server/issues/16934

If we start to move it around now, this will become even worse. See the Tasks app for example (sorry, it's always Tasks 🙈):

So, in case you really decide to do this, it will be a breaking change at least and other apps will need to overwrite the style as it doesn't fit their layout.

Here, is how it looks for Contacts (still has the old style toggle):

And here for an app with a Breadcrumb bar:

[marcoambrosini]

@raimund-schluessler I understand your concern but I think that the spacing of the toggle relative to other elements is off. We fixed that in talk and I'm trying to bring custom changes we have there upstream. Of course this would be a breaking change. @skjnldsv thoughts?

[raimund-schluessler]

@raimund-schluessler I understand your concern but I think that the spacing of the toggle relative to other elements is off. We fixed that in talk and I'm trying to bring custom changes we have there upstream. Of course this would be a breaking change. @skjnldsv thoughts?

The problem is that other apps will have to undo the custom changes of Talk because they won't work with their layout. So instead of having one custom CSS rule in Talk, we will have multiple (one in every other app instead).

[skjnldsv]

The problem is that other apps will have to undo the custom changes of Talk because they won't work with their layout

Yes, this is indeed not acceptable.
Either we have a dedicated variable that matches such spacing? (#1946 (comment)) Or we find something different? 🤷

[marcoambrosini]

@raimund-schluessler I think that other apps could benefit from a standardized 8px spacing too.
Looking at your screenshot, in tasks it seems that the toggle with the new margin is now almost aligned with the input field. With this change, If the input field had a height of 44px it could align perfectly with the toggle, similar to the avatar here:

[raimund-schluessler]

@raimund-schluessler I think that other apps could benefit from a standardized 8px spacing too.
Looking at your screenshot, in tasks it seems that the toggle with the new margin is now almost aligned with the input field. With this change, If the input field had a height of 44px it could align perfectly with the toggle, similar to the avatar here:

I don't say that standardization is bad, or that other apps won't benefit from having it aligned. But unless we find a proper space for the AppNavigationToggle in the layout which accounts for its presence, every app will still have to do custom changes to it's position.

For example, in case we adapt the files app to use the vue-components as well at some point, having an 8 px top spacing will just not work with the breadcrumb bar (since it doesn't have a padding at the top).

And on a side note: In my impression, the AppNavigationToggle shouldn't be aligned with any element in the content area, since it doesn't logically belong there (e.g. having it aligned with the input field in Tasks, suggests a logical relation to it, which does not exist). Having it top aligned with the AppNavigation pane is the proper solution imho, but that's just my taste.

[jancborchardt]

So my take just from the design / user perspective:

• I do think the toggle should be aligned with elements in the content area. This is similar to what Android and iOS do
• Yes it is not related to the content really, but it should still be visually pleasing, meaning e.g. vertically centered.
• We can talk about the details of how much exactly, if 8px or 4px
• It might be variable depending on the app – Talk’s top bar will be higher cause it has 2 lines. But the button should not stick to the top left like it does now, that just looks off.
• Yes we should standardize the units, we want to move to use bases of 4 so we can use that as a variable?

Regarding the specific app examples:

• In Talk it looks great, as shown
• In Contacts I think we can align it with a button, which should be 44px high as well and will have some distance from the top too, as per https://github.com/nextcloud/server/issues/16934#issuecomment-542201187
• Possibly same in Mail
• Tasks could be similar, as per your screenshot there’s very little to adjust @raimund-schluessler – what Android does is that the navigation toggle is actually inside the search field on the left.
• In Files and other apps with the breadcrumbs bar, it’s also not bad to have the breadcrumbs bar a bit larger

[raimund-schluessler]

• In Files and other apps with the breadcrumbs bar, it’s also not bad to have the breadcrumbs bar a bit larger

Altough I think more spacing above the breadcrumb bar is not really necessary, it's fine with me. But could we then maybe only use 4px top like in this screenshot? 8px would look quite weird I think.

4px:

8px:

[raimund-schluessler]

• Tasks could be similar, as per your screenshot there’s very little to adjust @raimund-schluessler – what Android does is that the navigation toggle is actually inside the search field on the left.

I don't have a problem adjusting the layout. I just understood it so that you want to have the same button position for all apps. And I am a bit skeptical that this will really work out well. Mainly because many apps don't position the AppNavigationToggle button well just as it is located now. And moving it further into the content will make this problem even worse (see e.g. Contacts or Calendar).

[jancborchardt]

@raimund-schluessler your example looks great! :)

And yeah, the positioning probably doesn’t have to be exactly the same in all apps – but in no app should it stick to the very top left and be unaligned to any content, like it is now.

[marcoambrosini]

Let's go with 4 then, I'll reduce talk's topbar height

[skjnldsv]

Ok, so var(--top-bar-spacing) @skjnldsv ??

🤷

[skjnldsv]

Hum, shouldn't those also be available as a css var within content?
We should maybe start doing this (add them to the Content so anything within can use it for example?)

[marcoambrosini]

ping @skjnldsv

[marcoambrosini]

What should we do here????

[jancborchardt]

Ok, so var(--top-bar-spacing) @skjnldsv ??

Isn’t that a good approach as you suggested?

[skjnldsv]

I would like this new css variable to be documented tbh

[jancborchardt]

By the way @raimund-schluessler, with the new breadcrumb component with bigger visual elements, maybe the 8px spacing also makes sense for Files now? The slightly more whitespace is definitely nice, and then it would fit for Talk, Files and probably others too.

[marcoambrosini]

I would like this new css variable to be documented tbh

@skjnldsv where should we add the docs? To this file?

[skjnldsv]

I would like this new css variable to be documented tbh

@skjnldsv where should we add the docs? To this file?

Very good question :)
Maybe we need to dedicated design system documentation.
There was the old one in the docs, but it's proably not very used nor up to date anymore.
We could add a dedicated component-related one here? 🤔

At the very least, I would add that to the AppContent documentation

[raimund-schluessler]

How should we proceed here? We are heading for a new major release and it would be good to have this included, I think.

[marcoambrosini]

Added a couple of lines of docs in the docs section and in the css section where the variable is declared, let's get this in?

[raimund-schluessler]

Added a couple of lines of docs in the docs section and in the css section where the variable is declared, let's get this in?

Fine with me. Apps will still need to adjust the position to make it work with their layout, though.