Fix the space between navigation icon and label

[GretaD]

there's a lot of space between the icon and the label. On mail app that doesnt look good when we have more than one level submailbox. Even though we removed the option to add more than one level submailbox, the people who added more than one level before we removed it, have problem reading the name of their mailboxes.

before

after

[raimund-schluessler]

It would also help to correctly align the three dot menus on the right.

[GretaD]

It would also help to correctly align the three dot menus on the right.

and with this you mean to have it always on the right, and not to depend on the mailbox label?

[ChristophWurst]

#1808 was merged and so any change on master is going to be 5.x. we have to backport this

[raimund-schluessler]

Please see https://github.com/nextcloud/nextcloud-vue/pull/2343/files#r746616031

[raimund-schluessler]

@GretaD How do you even get more than one level nesting with AppNavigationItem? The component actively prevents that here:
https://github.com/nextcloud/nextcloud-vue/blob/063cb379aae4cf13ab949d1411fad9cf884cb242/src/components/AppNavigationItem/AppNavigationItem.vue#L378-L386

[GretaD]

@GretaD How do you even get more than one level nesting with AppNavigationItem? The component actively prevents that here:

Remove the && top here: https://github.com/nextcloud/mail/blob/e2978c668d11ad2a190bc372315e3d6a11444239/src/components/NavigationMailbox.vue#L58
just for testing

[raimund-schluessler]

And a quick review from @jancborchardt, @skjnldsv or @nimishavijay would be good, since it affects the global layout a bit and I am not the one to judge what is intended there.

[GretaD]

And a quick review from @jancborchardt, @skjnldsv or @nimishavijay would be good, since it affects the global layout a bit and I am not the one to judge what is intended there.

We had a call with Nimisha, so she is aware, but would be nice to have a final look.

[dartcafe]

Is it necessary to indent every level with the width of the icon width? Maybe we could reduce it to 10 or 12px?
current indentation:

12px:

[nimishavijay]

Looks really nice here, but since this a change in the component itself can we be sure that other apps are not affected? Eg. Deck

[raimund-schluessler]

Looks really nice here, but since this a change in the component itself can we be sure that other apps are not affected? Eg. Deck

They sure will be affected. Every app which uses AppNavigationItem and AppNavigationSettings will see this change.

[GretaD]

Looks really nice here, but since this a change in the component itself can we be sure that other apps are not affected? Eg. Deck

All apps that use this component are affected but the scenario with more than one children is only mail(IIRC). The space between icon and label is very wide everywhere, thats why we did it here.

[skjnldsv]

Looks really nice here, but since this a change in the component itself can we be sure that other apps are not affected? Eg. Deck

Every app will be affected. That's why it needs approval from the Designer team 😉
This is a hard change in the way we've been doing things over the last 5 years.
Non-vue components are also not using such layout.

cc @jancborchardt

[ChristophWurst]

12px:

I also still think that the space between icon and text looks a bit too big. Reducing that would give us some more space for long text.

[ChristophWurst]

Every app will be affected. That's why it needs approval from the Designer team wink

@jancborchardt please also tell us if you consider this more of a design change or a bugfix. Only if the old intention was a design bug I would backport. Otherwise this should only come with v5.0 and marked as breaking design change. Not breaking in the sense of the old app code needs to be adjusted but that the component now renders differently and app devs have to decide what versions of Nextcloud they want to ship this.

[GretaD]

12px:

I also still think that the space between icon and text looks a bit too big. Reducing that would give us some more space for long text.

this screenshot doesnt have my changes, check the one on the ticket. The result you see now its the space that the icon "forces" because its 44h and 44w. I am not sure if we should change that.

[nimishavijay]

Many apps have only one sublevel of hierarchy, no? (Files, Deck, Notes) In that case less indentation would look odd. This is kind of an edge case in Mail.

Would it be possible to enforce this reduced indentation only when there are more than 3 sub levels? What do you think? cc @jancborchardt

[GretaD]

Many apps have only one sublevel of hierarchy, no? (Files, Deck, Notes) In that case less indentation would look odd. This is kind of an edge case in Mail.

Files dont use the component, but i tested it with Deck and it looks very nice. I dont have a screenshot, but try to test it and see. No its not an edge case, icon is too far from the text starting from the main level and then to the children. So it will look odd if we change it only for mail IMO.

[jancborchardt]

The change makes sense and @GretaD’s fix & screenshot looks good – cause the folder icon of the subentry is exactly between the icon and text of the parent.

It is also ok to do it as a general change for every app. Additionally, cause Files is the only of the "Hub" apps which doesn’t use the components yet, could the change also be done there so it looks common?

If it should be backported or not I would say is your call @ChristophWurst.

[skjnldsv]

could the change also be done there so it looks common

Please open a ticket on server for it :)