Fix appnavigationitem layout

[marcoambrosini]

Fix #1868 and adds 4px padding on top and bottom too so that the actions hover circle doesn't intersect with the boundary of the element

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

[marcoambrosini]

@raimund-schluessler I added the extra margin on the right of the counter for a visual balance, the element is much shorter than the actions component and it looked weird too close to the right border of the wrapper.

I can come up with a different classname though, what about ...__counter-wrapper?

[raimund-schluessler]

@raimund-schluessler I added the extra margin on the right of the counter for a visual balance, the element is much shorter than the actions component and it looked weird too close to the right border of the wrapper.

The thing is that the counter can have different widths. So setting a fixed margin will only achieve a "balance" for a single width. In all other cases, it won't be balanced and not aligned at the right side. That's why I would prefer a right alignment.

[raimund-schluessler]

I can come up with a different classname though, what about ...__counter-wrapper?

A wrapper won't be necessary if we just use &::v-deep .app-navigation-entry__counter.

[marcoambrosini]

A wrapper won't be necessary if we just use &::v-deep .app-navigation-entry__counter.

The thing is that appnavigationcounter will be deprecated and developers will start using this instead, so we'd have to v:deep two class names with the same rule

[marcoambrosini]

The thing is that the counter can have different widths. So setting a fixed margin will only achieve a "balance" for a single width. In all other cases, it won't be balanced and not aligned at the right side. That's why I would prefer a right alignment.

About this, the action's hover feedback is visible only when hovering, most of the times you only see the 3 dots icon, so the spacing of the element to the right is even bigger.

This is what happens without that extra padding and that's what I'm trying to tackle here. It's sort of independent of the amount of characters in the counter, but the less of them, the more out of balance it looks.

[raimund-schluessler]

A wrapper won't be necessary if we just use &::v-deep .app-navigation-entry__counter.

The thing is that appnavigationcounter will be deprecated and developers will start using this instead, so we'd have to v:deep two class names with the same rule

Ah, yeah, fair enough. Then let's just use a different class name.

[raimund-schluessler]

This is what happens without that extra padding and that's what I'm trying to tackle here. It's sort of independent of the amount of characters in the counter, but the less of them, the more out of balance it looks.

This is how it looks with longer counters when right-aligned:

And the same elements with the changes from this PR:

I agree the differences are subtle and probably a matter of taste. I just like alignment 😉 I leave it up to you.

[skjnldsv]

/backport to stable3

[raimund-schluessler]

@ma12-co @skjnldsv This change (the wrapper around the counter) breaks the Calendar and the Tasks app, since both apps use the counter slot to show an avatar and the sharing action like so:
https://github.com/nextcloud/calendar/blob/master/src/components/AppNavigation/CalendarList/CalendarListItem.vue#L39-L45

I am not really sure what is the proper solution here. The counter slot probably wasn't supposed to show an Avatar and an ActionItem in the first place. But there is no other slot to show something in the app-navigation-entry__utils element.
For now, adding

.app-navigation-entry__counter-wrapper {
	display: flex;
	align-items: center;
	flex: 0 1 auto;
}

mitigates the issue:

Also @ChristophWurst, because you will have the same problem in the Calendar app.

[raimund-schluessler]

Also, increasing the height of the AppNavigationItem by 20% is to much, I think. Was this discussed with @nextcloud/designers and especially @jancborchardt? In my impression, we now waste a lot of screen space (especially height) in the AppNavigation, because every item is now 52px instead of 44px high.

Before we see 15+ entries:

After we only see 13 entries without a real benefit imho:

I will overwrite this change in the Tasks app for now, until this is discussed and confirmed.

[skjnldsv]

The counter slot probably wasn't supposed to show an Avatar and an ActionItem in the first place.

Yep 😞

Also, increasing the height of the AppNavigationItem by 20% is to much

Ah right @ma12-co, this is supposed to be a bugfix. This is too much, please let's keep the 44px, this is not a feature-pr :)

[skjnldsv]

@ma12-co please only fix the margin :)