Fit app-navigation to css guidelines by skjnldsv · Pull Request #521 · nextcloud/mail

skjnldsv · 2017-09-13T11:29:55Z

jancborchardt · 2017-09-15T10:30:55Z

Oh by the way, cause I see this (I know that’s not how it’s supposed to look like ;):

But the one thing is: The folders are indented sub-entries of the accounts. That indentation is stylistically not something which we would want in this case though. :\ The account is just a heading, but not a »folder« per se.

skjnldsv · 2017-09-15T12:31:24Z

@jancborchardt, it's an easy fix with a single line on the mail app css. That being said, we should wait the end of my work here to really see what it would look like! :)

jancborchardt · 2017-09-15T14:00:14Z

Okidoke! I just snook a peek :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2017-09-20T16:42:23Z

@jancborchardt Feeling better? 😉

jancborchardt · 2017-09-21T16:10:10Z

@skjnldsv not yet – this would be the correct indentation ;) As it is in the current design too. That’s why I mentioned it’s unusual, but we don’t want to have it look like a strange tree view by default.

skjnldsv · 2017-09-21T16:12:17Z

I disagree, folders are subitems of the account, they should appear as indented.

jancborchardt · 2017-09-21T16:17:14Z

The folders are not subitems of an account. The account name is simply the header of the set of folders. :)
I’m pretty particular about this since the tree view look by default makes the app look quite old-style. Even when you only have a single account, the folders will be indented and that just looks strange.

What do you think @ChristophWurst @Gomez?

skjnldsv · 2017-09-21T16:20:14Z

I'm okay with both! :)
I was just stating how the app outputs the divs! 😉

If we go for NOT a subitem, the work will be far more considerable though.

pixelipo · 2017-09-21T16:30:27Z

I agree with @jancborchardt regarding indentation. Account name is not clickable and it's not collapsible, hence it shouldn't pretend to be a parent folder.

However, as far as headers go, account name could be styled better. Right now, it's styled to like like a folder (mailbox). It's very hard to differentiate it from actual mailboxes (inbox, sent etc.) App-navbar doesn't seem to support headers at the moment, so I don't know how it should look :)

pixelipo · 2017-09-21T16:35:31Z

@@ -1,32 +1,35 @@
 {{#if emailAddress}}
-<div class="mail-account-color" style="background-color: {{accountColor emailAddress}}"></div>
+<div class="mail-account-color app-navigation-entry-bullet" style="background-color: {{accountColor emailAddress}}"></div>


Shouldn't this be rendered via CSS, similar to the way folders' icons are done?

You mean the background color?

Not only that, the whole .mail-account-color could be moved over to .mail-account-email:before. That would make for a cleaner DOM.

edit: Because the bullet is just decoration

The bullet is now integrated into server. It's part of the css guidelines.
see nextcloud/server#6399

jancborchardt · 2017-09-21T20:14:00Z

However, as far as headers go, account name could be styled better. Right now, it's styled to like like a folder (mailbox). It's very hard to differentiate it from actual mailboxes (inbox, sent etc.) App-navbar doesn't seem to support headers at the moment, so I don't know how it should look :)

It’s greyed out for now, and has the colored circle. That’s what differentiates it from the folders – any other ideas? :)

@skjnldsv so should we in the mail app simply override that one first indent? Or how to do it? :) I mean, semantically the accounts are not a »parent folder« of the mail folders, as @pixelipo also said, so they shouldn’t be like that in the markup.

skjnldsv · 2017-09-21T20:25:46Z

If we start overriding the default server, let's drop this full pr. I don't see the point. :)
Speaking of hierarchy, I find the indentation pretty logical.
But I also see your point. We could put all items as first childs. @ChristophWurst is this an easy task?

pixelipo · 2017-09-21T21:07:01Z

@skjnldsv Well I think we need to decide whether we want to support list headings in the app navbar. In my opinion, it's logical to have this - semantically, list usually have titles.

No app that I know of (apart from Mail) has the need for them - so far, but if you look at Calendar, you will notice that regular and webcals are separated and the "New ***" button is acting as a heading:

I would also like to attach Geary sidebar - they are indenting folders within accounts, even though accounts themselves are not clickable (they are collapsible, though).

skjnldsv · 2017-09-21T21:24:13Z

Thanks @pixelipo! :)
My opinion is on the subfolder side. I like the hierarchy it induce and it's easier to have it so we can easily collapse what we don't want to see.

jancborchardt · 2017-09-22T09:23:48Z

@pixelipo and that sidebar look is precisely what I find annoying with Geary. :) It’s complicating the view and makes the structure look more complex than it actually is.

jancborchardt · 2017-09-22T09:26:32Z

@skjnldsv that’s the thing though – the Mail app doesn’t need to override it if the accounts are headers and not parent folders. We could add a bit of vertical whitespace like in the Calendar too, to make the separation more clear. (Note that the buttons being used as headings in the Calendar is by design.)

Also the mail app automatically collapses stuff already, like all the folders which are secondary. (Except inbox, sent, drafts, favorites.) You already can easily see 2–3 accounts in the list without scrolling. Being able to collapse the whole account just introduces a 2-step process.

Sorry if it sounds like I’m arguing against standardization – in certain respects the design just needs to be adjusted to be more seamless for the specific app we are looking at. And apps like Mail and Calendar are designed that way for a reason. The goal of standardizing the CSS is not to put everything down to a lowest common denominator. ;)

skjnldsv · 2017-09-22T10:00:25Z

I'm OK with you @jancborchardt! :)
I was saying that in case if putting folders as first level entries is not possible. Because removing the ul padding is not the proper way to do it! :(

jancborchardt · 2017-09-28T11:32:55Z

The core of the indentation problem is that the grouping of the account sets are also marked up as ul. If we just do that via div, it would solve the problem, no? Because only the accounts themselves should be ul, with the entries Inbox, Sent, Drafts etc being the li.

ChristophWurst · 2017-09-28T12:09:22Z

Removed unwanted attribute to all inbox

It's about all the attributes you've added. They don't belong to the account but to the actual folder: the unified inbox. I will take a closer look next week - promised!

skjnldsv · 2017-09-28T13:59:09Z

The heading bar of the Settings drawer is half-transparent when opened:

Nice callout, this is a core bug!

The active folder (»Sent« in this case) doesn’t have the blue bar on the left:

Because it's a sub item :)

How do we now fix the »everything is indented by default« issue? :)

Good question. I'm not sure we can put everything as first items. This would require a big change in the backbone/marionette controllers! :/

ChristophWurst · 2017-10-03T16:59:17Z

Regarding

@skjnldsv Well I think we need to decide whether we want to support list headings in the app navbar. In my opinion, it's logical to have this - semantically, list usually have titles.

and

Good question. I'm not sure we can put everything as first items. This would require a big change in the backbone/marionette controllers! :/

IMO of course, folders belong to an account and thus reflect a hierarchy. However, I'm not a big fan of indenting that in the sidebar as that seems like a lot of wasted space to me. I like the idea of having list heading/separators that we could use to separate different accounts. @skjnldsv is something like that feasible? Like something that could be used by any app somehow? I image that as one entry in the list with a dedicated class which makes the font a bit stronger/darker/whatsoever. We wouldn't then add it for the unified account of course, but rather for the specific ones only.

I'd gladly implement it for this app, I just have to know how to structure the HTML and I'll fight with/against backbone/marionette to achieve that 😉

A rough structure would look like this

Sidebar
- Unified inbox
- Account 1 title heading
- Inbox
- Sent
- Other
  - Subfolder
  - Subfolder.Subfolder
- Account 2 title heading
- Inbox
- Sent

To comply with the current design, list headings should have the ability to show an icon on the left (like the other entries in the list, I think we have the styling in place already). As before, we wouldn't go deeper than one subfolder.

Does that make sense? :)

ChristophWurst · 2017-10-09T09:33:00Z

In addition to my comment above, this would be the rough HTML structure:

<div id="app-navigation">
    <ul>
        <li>
            <a href="#">All inboxes</a>
        </li>
        <li class="heading">
            <a href="#">user1@domain.tld</a>
        </li>
        <li>
            <a href="#">Inbox</a>
        </li>
        <li>
            <a href="#">Drafts</a>
        </li>
        <li>
            <a href="#">Sent</a>
        </li>
        <li>
            <a href="#">Folder with sub folders</a>
            <ul>
                <li><a href="#">Subfolder 1</a></li>
                <li><a href="#">Subfolder 2</a></li>
            </ul>
        </li>
        <li class="heading">
            <a href="#">user2@domain.tld</a>
        </li>
        <li>
            <a href="#">Inbox</a>
        </li>
        <li>
            <a href="#">Drafts</a>
        </li>
        <li>
            <a href="#">Sent</a>
        </li>
    </ul>
</div>

skjnldsv · 2017-10-09T10:12:29Z

That is perfect!!

ChristophWurst · 2017-10-09T11:10:03Z

That is perfect!!

Awesome. Then let me try to adjust our views to generate a HTML closely to what I've sketched above. Could be that Marionette requires us to have some additional elements due its requirement of an surrounding element and our nested view hierarchy. Should be fine though :)

skjnldsv · 2017-10-09T12:56:38Z

@ChristophWurst good luck. The div wrapping is a pain!! :(

ChristophWurst · 2017-10-10T08:32:16Z

@ChristophWurst good luck. The div wrapping is a pain!! :(

Indeed they are. And from experience I can tell it's super hard to get rid of them without introducing random bugs. Thus I'd like to ask whether it would be possible to relax the CSS rules a bit to be a bit less strict about the HTML structure. As in: ignore some wrapping divs.

Something like

 core/css/apps.scss | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/core/css/apps.scss b/core/css/apps.scss
index 9978c38033..fa984ef90b 100644
--- a/core/css/apps.scss
+++ b/core/css/apps.scss
@@ -104,7 +104,7 @@ kbd {
 		box-sizing: border-box;
 		display: flex;
 		flex-direction: column;
-		> li {
+		li {
 			display: inline-flex;
 			flex-wrap: wrap;
 			order: 1;
@@ -201,8 +201,8 @@ kbd {
 			}
 		}
 		/* Menu and submenu */
-		> li,
-		> li > ul > li {
+		li,
+		li > ul > li {
 			position: relative;
 			width: 100%;
 			box-sizing: border-box;

seems to allow styling Mail according to the guidelines while still using some additional divs. @skjnldsv how bad is that? 😨

ChristophWurst · 2017-10-10T08:59:36Z

@skjnldsv how bad is that? 😨

To answer my own question: very bad. Now the stlye is applied to any li down the nested structure. Is there any fancy CSS trick to loosen the selector but still limit it to the first level of lis? 🤔

ChristophWurst · 2017-10-10T09:06:38Z

Although it still looks a bit odd, I seem to be on the right track

ChristophWurst · 2017-10-10T09:24:03Z

Putting this on hold until #521 (comment) has been answered. So far, so good

ChristophWurst · 2017-10-10T11:00:58Z

@skjnldsv please see #565 😉

skjnldsv · 2017-10-10T11:02:35Z

It's done on purpose to force the correct css guidelines. :)
So no, it would be counter productive to loosen the selector and basically allow anything to be done on the app navigation.

ChristophWurst · 2017-10-10T11:07:57Z

It's done on purpose to force the correct css guidelines. :)
So no, it would be counter productive to loosen the selector and basically allow anything to be done on the app navigation.

I see. Unfortunately, that means I'm out of idea on how to solve this. I definitely won't merge all the views into one to remove the wrapping divs and I also won't work on hacks to remove the existing divs. With the tools we currently use (Backbone+Marionette), it seems very hard to obey those strict rules. And a switch to another front-end framework is not planned right now as it would require a lot of effort.

ChristophWurst · 2017-10-10T11:36:51Z

2e723e9 removes two unnecessary wrapping divs. The other ones cannot be removed that easily it seems.

skjnldsv · 2017-10-10T14:09:47Z

I'll think about what we can do on mail. On the other hand, it seems incredible that a very used engine like backbone forces users to wrap in div... I don't get their idea on this. 🤔

What is the main problem with your latest changes @ChristophWurst? The screenshot seems great?

ChristophWurst · 2017-10-10T14:22:01Z

What is the main problem with your latest changes @ChristophWurst? The screenshot seems great?

Only because I've adjusted the server css with the patch from above 🙈

On the other hand, it seems incredible that a very used engine like backbone forces users to wrap in div... I don't get their idea on this. 🤔

I guess jashkenas/backbone#546 (comment)

jancborchardt · 2017-10-10T14:53:04Z

Looks very good so far! :) Much better

ChristophWurst · 2017-11-02T19:09:46Z

I assume we can close this as #565 and #594 have been merged :)

lock · 2018-11-20T18:49:03Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

skjnldsv added 2. developing technical debt labels Sep 13, 2017

skjnldsv self-assigned this Sep 13, 2017

skjnldsv changed the title ~~Initial fix for app-navigation~~ Fit app-navigation to css guidelines Sep 13, 2017

ChristophWurst requested a review from jancborchardt September 13, 2017 12:47

jancborchardt requested review from ChristophWurst, Gomez and lcalaresu September 13, 2017 13:50

skjnldsv added 2 commits September 19, 2017 19:44

Initial fix for app-navigation

f643c4d

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fix inline js

a54cbab

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the app-navigation-css-guidelines branch from 6d4ac2d to a54cbab Compare September 19, 2017 17:44

skjnldsv added 2 commits September 20, 2017 07:36

Fix wrap to parent

93183d3

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Css fixes for bullet

b41ea7a

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

pixelipo reviewed Sep 21, 2017

View reviewed changes

ChristophWurst self-assigned this Sep 28, 2017

ChristophWurst added 1. to develop and removed 2. developing labels Oct 9, 2017

ChristophWurst mentioned this pull request Oct 10, 2017

Clean up navigation views #565

Merged

ChristophWurst closed this Nov 2, 2017

ChristophWurst deleted the app-navigation-css-guidelines branch November 2, 2017 19:09

ChristophWurst unassigned ChristophWurst and skjnldsv Nov 2, 2017

lock Bot locked and limited conversation to collaborators Nov 20, 2018

Uh oh!

Conversation

skjnldsv commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Sep 15, 2017

Uh oh!

skjnldsv commented Sep 15, 2017

Uh oh!

jancborchardt commented Sep 15, 2017

Uh oh!

skjnldsv commented Sep 20, 2017

Uh oh!

jancborchardt commented Sep 21, 2017

Uh oh!

skjnldsv commented Sep 21, 2017

Uh oh!

jancborchardt commented Sep 21, 2017

Uh oh!

skjnldsv commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pixelipo commented Sep 21, 2017

Uh oh!

pixelipo Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

pixelipo Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

jancborchardt commented Sep 21, 2017

Uh oh!

skjnldsv commented Sep 21, 2017

Uh oh!

pixelipo commented Sep 21, 2017

Uh oh!

skjnldsv commented Sep 21, 2017

Uh oh!

jancborchardt commented Sep 22, 2017

Uh oh!

jancborchardt commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Sep 22, 2017

Uh oh!

jancborchardt commented Sep 28, 2017

Uh oh!

ChristophWurst commented Sep 28, 2017

Uh oh!

skjnldsv commented Sep 28, 2017

Uh oh!

ChristophWurst commented Oct 3, 2017

Uh oh!

ChristophWurst commented Oct 9, 2017

Uh oh!

skjnldsv commented Oct 9, 2017

Uh oh!

ChristophWurst commented Oct 9, 2017

Uh oh!

skjnldsv commented Oct 9, 2017

Uh oh!

ChristophWurst commented Oct 10, 2017

Uh oh!

ChristophWurst commented Oct 10, 2017

Uh oh!

ChristophWurst commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst commented Oct 10, 2017

Uh oh!

ChristophWurst commented Oct 10, 2017

Uh oh!

skjnldsv commented Oct 10, 2017

skjnldsv commented Sep 13, 2017 •

edited

Loading

skjnldsv commented Sep 21, 2017 •

edited

Loading

pixelipo Sep 21, 2017 •

edited

Loading

jancborchardt commented Sep 22, 2017 •

edited

Loading

ChristophWurst commented Oct 10, 2017 •

edited

Loading