Toggle app navigation not only on mobile, but on desktop as well #8620

jancborchardt · 2014-05-16T14:16:24Z

Several reasons for this:

We now have a sidebar in Files, so there’s two sidebars taking space (most other apps have a sidebar already).
The app navigation is arguably not needed all the time.
If you have many apps installed, scrolling on the app navigation is clunky and gets ugly fast (too thin)
If you don’t have many apps installed, it looks like a waste of space (like for the Enterprise Edition)
We give apps the whole width, which alleviates a lot of issues we had in the past with CSS and sizing

karlitschek · 2014-05-16T14:45:17Z

very nice 👍
Now the only thing that is missing is the dedicated apps menu on top.

jancborchardt · 2014-05-16T15:28:49Z

Ok, I added the app name next to the logo. We probably need to tweak the look. Like this, big like the height of the »d« of ownCloud it looks ok. Better than small when it fits the x-height of the ownCloud logo – especially because it then is too low down.
I wanted to look into using a proper webfont with ownCloud (like Source Sans Pro), so we can control the font weights better. Maybe this is the time.

(Regarding the change that on desktop, clicking the ownCloud logo should go to »home« (Files), I would need help from @PVince81 – probably putting that media query back in the JS.)
BUT I really think it would be better if clicking on both logo and app name would trigger the menu – otherwise there will be confusion about which triggers what. Also, »Files« is always on the very top left, so very easy to reach.

jancborchardt · 2014-05-16T15:47:00Z

(Fixed an overflow issue with the navigation I found while testing on iPod touch – should be fixed for iPhone/iPod now.)

jancborchardt · 2014-05-16T15:47:16Z

Please review @owncloud/designers :)

jancborchardt · 2014-05-16T16:29:38Z

@PVince81 I know what’s wrong with the tests – they expect $navigation.is(':visible')) to be false. But it’s not false, because it’s slid up and not hidden. But I don’t know how to fix them – can you help?

PVince81 · 2014-05-16T16:51:53Z

If the "slide up" navigation has a specific recognizable class you could try changing it to $navigation.hasClass('slideup-class') instead.
I don't like is(':visible') in general because sometimes unit tests use invisible elements when testing, so that will always fail. I recommend always using the hidden class, or in your case another name.

PVince81 · 2014-05-16T16:53:26Z

Hmmm, maybe you can simply remove the obsolete unit tests.
If I understand well, the menu is now always inside the logo, so no need to check for toggling any more.

jancborchardt · 2014-05-16T17:22:13Z

@PVince81 can you maybe do this – you created the tests, right? I don’t really see which ones are obsolete and which ones are still needed.

PVince81 · 2014-05-17T16:04:14Z

I've removed the obsolete code, tests should pass now.

MorrisJobke · 2014-05-19T13:33:42Z

core/css/header.css

+	position: relative;
+	color: #fff;
+	font-size: 26px;
+	opacity: .4;


Drop that opacity (unused)

MorrisJobke · 2014-05-19T13:46:16Z

I added OC.menuSpeed which controls the animation speed for the menu toggle. I would vote for 100 rather than 200. If you want to test just type into the JS console OC.menuSpeed=WHATEVER and the speed got applied.

MorrisJobke · 2014-05-19T13:52:55Z

@jancborchardt We don't need the push item anymore, didn't we?

karlitschek · 2014-05-19T14:02:16Z

Hmm. The apps name looks a bit strange. Perhaps a bit too big. Of course I have no better idea for a better design. :-)

MorrisJobke · 2014-05-19T14:11:58Z

@karlitschek The font doesn't fit ... maybe we use the ownCloud font for this?

MorrisJobke · 2014-05-19T14:25:43Z

Now:

Open Sans Condensed:

See this as a first try: Maybe we can find a better solution.

schiessle · 2014-05-19T16:07:50Z

@MorrisJobke It looks already much better!

But personally I'm not sure if it is possible to make it look really good with the two strings next to each other. I wonder if it is really needed if the logo doesn't link to a different location than the app name (and I agree with @jancborchardt that this could be confusing). Maybe we should just drop the idea and don't display the app name at all? Or a different idea: remove the "ownCloud" string from the logo and just show the cloud together with the app name? This could look nice too.

jbtbnl · 2014-05-19T16:24:45Z

I like the suggestion by @schiesbn to hide "ownCloud" and only display the ownCloud icon. It would be quite similar to my mock-ups in #8159 (comment).

Additionally we could show "ownCloud" instead of the app name when the menu is opened (with a subtle fade-in).

jancborchardt · 2014-05-19T17:26:34Z

@MorrisJobke I will prepare a pull request to use Source Sans Pro as the general font for the ownCloud interface – wanted to do that anyway. I’ll fix the appname there as we a) shouldn’t introduce a font just for that part and b) it’s a separate topic.

@schiesbn I’m also fully ok with not displaying the app name at all. Maybe that will make the menu harder to discover though.

@jbtbnl naah – we won’t use images as text. ;)

@schiesbn @jbtbnl we shouldn’t change the icon which is displayed: It will require an additional logo for themes and existing ones will break.

jancborchardt · 2014-05-28T09:35:17Z

Reduced the font size on the appname in the header, looks much better now.

Please review @karlitschek @owncloud/designers

jbtbnl · 2014-05-28T09:53:36Z

core/css/header.css

+#navigation {
+	top: 45px;
+	bottom: initial;
+	width: 265px;


3 * 80px + 2 * 5px = 250px, not 265px

It was like that before, but that broke it on mobile and showed only 2 columns. Remember there will be a scrollbar shown as well. :)

(Ref: 15e9081 )

It still looks bad though ;)

Seriously? Doesn’t look so bad to me. Slight spacing issue, but not jarring.

Let’s do it like this for now, we can check more sophisticated fixed in follow-up pull requests.

jbtbnl · 2014-05-28T10:59:35Z

👍

jancborchardt · 2014-05-28T11:01:11Z

@karlitschek what do you say to the improved app name display?

karlitschek · 2014-05-28T11:07:02Z

Looks much better! What do you think about making a click on the ownCloud logo a "home" action that always goes to / on files ?

jancborchardt · 2014-05-28T11:10:00Z

I still think that making a difference between clicking on the ownCloud logo and the appname part will be highly confusing. It will be even worse on mobile because of the little space we have. Files will always be in the top left so it’s quick to reach.

karlitschek · 2014-05-28T11:11:35Z

O.K. give me a few more minutes to test and discuss!

ghost · 2014-06-04T14:51:32Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5321/

ghost · 2014-06-04T15:37:20Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5326/

nickvergessen · 2014-06-04T15:44:03Z

If you install an app, it previously was added to the navigation with js.
Now you need to reload the page, before the app is listed

MorrisJobke · 2014-06-04T16:23:36Z

@nickvergessen Your issue is fixed with the latest commit

👍 from my side for the other changes

jancborchardt · 2014-06-04T16:28:50Z

Ok, let’s wait for Jenkins and then merge.

I’m fine with the »Clicking logo goes to Files« for now. We can still see how it fares and what feedback we get. That decision is not a blocker for merging though. :)

PVince81 · 2014-06-04T17:03:30Z

Looks good 👍

ghost · 2014-06-04T21:54:26Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5341/

scrutinizer-notifier · 2014-06-04T22:41:04Z

A new inspection was created.

MorrisJobke · 2014-06-04T22:41:14Z

@PVince81 We have to look at the other failing test. I have no clue why it fails. The second failing test is fixed in the above commit.

scrutinizer-notifier · 2014-06-04T23:55:34Z

The inspection completed: 4 new issues

ghost · 2014-06-05T02:09:21Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5357/

scrutinizer-notifier · 2014-06-05T07:46:31Z

A new inspection was created.

ghost · 2014-06-05T08:34:02Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5366/

scrutinizer-notifier · 2014-06-05T08:52:22Z

A new inspection was created.

DeepDiver1975 · 2014-06-05T08:52:50Z

failing unit test is unrelated -> merge

Toggle app navigation not only on mobile, but on desktop as well

jancborchardt · 2014-06-05T08:55:53Z

YEAH! 🚀 now we can continue with the mobile stuff: #8159

Thanks to everyone

jancborchardt added Design labels May 16, 2014

jancborchardt mentioned this pull request May 16, 2014

Adds Animation to Toggling Navigation on < 768px widths #8278

Closed

MorrisJobke reviewed May 19, 2014
View reviewed changes

core/css/header.css

position: relative;

color: #fff;

font-size: 26px;

opacity: .4;

Copy link

Contributor

MorrisJobke May 19, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop that opacity (unused)

MorrisJobke added the 4 - Developing label May 21, 2014

jbtbnl reviewed May 28, 2014
View reviewed changes

add to app navigation after enabling

210a1ef

MorrisJobke added 5 - To review and removed 4 - Developing labels Jun 4, 2014

jancborchardt mentioned this pull request Jun 4, 2014

Mobile sidebar swipe #8159

Merged

7 tasks

fix first failing test

e3cbcad

fix unit test template

e186871

fix leading decimal Scrutinizer coding style issue

218131d

DeepDiver1975 added a commit that referenced this pull request Jun 5, 2014

Merge pull request #8620 from owncloud/design-navigation-two

1c20c72

Toggle app navigation not only on mobile, but on desktop as well

DeepDiver1975 merged commit 1c20c72 into master Jun 5, 2014

DeepDiver1975 deleted the design-navigation-two branch June 5, 2014 08:53

This was referenced Jun 5, 2014

Design forward: navigation & user menu #1932

Closed

Firefox , scroll bars, truncated icons and text #6691

Closed

MorrisJobke mentioned this pull request Jul 28, 2014

[OC 7.0.0] Cloud-Icon top left should react like current app title #9956

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 25, 2019

Toggle app navigation not only on mobile, but on desktop as well #8620

Toggle app navigation not only on mobile, but on desktop as well #8620

Conversation

jancborchardt commented May 16, 2014

karlitschek commented May 16, 2014

jancborchardt commented May 16, 2014

jancborchardt commented May 16, 2014

jancborchardt commented May 16, 2014

jancborchardt commented May 16, 2014

PVince81 commented May 16, 2014

PVince81 commented May 16, 2014

jancborchardt commented May 16, 2014

PVince81 commented May 17, 2014

MorrisJobke May 19, 2014

Choose a reason for hiding this comment

MorrisJobke commented May 19, 2014

MorrisJobke commented May 19, 2014

karlitschek commented May 19, 2014

MorrisJobke commented May 19, 2014

MorrisJobke commented May 19, 2014

schiessle commented May 19, 2014

jbtbnl commented May 19, 2014

jancborchardt commented May 19, 2014

jancborchardt commented May 28, 2014

jbtbnl May 28, 2014

Choose a reason for hiding this comment

jancborchardt May 28, 2014

Choose a reason for hiding this comment

jbtbnl May 28, 2014

Choose a reason for hiding this comment

jancborchardt May 28, 2014

Choose a reason for hiding this comment

jbtbnl commented May 28, 2014

jancborchardt commented May 28, 2014

karlitschek commented May 28, 2014

jancborchardt commented May 28, 2014

karlitschek commented May 28, 2014

ghost commented Jun 4, 2014

ghost commented Jun 4, 2014

nickvergessen commented Jun 4, 2014

MorrisJobke commented Jun 4, 2014

jancborchardt commented Jun 4, 2014

PVince81 commented Jun 4, 2014

ghost commented Jun 4, 2014

scrutinizer-notifier commented Jun 4, 2014

MorrisJobke commented Jun 4, 2014

scrutinizer-notifier commented Jun 4, 2014

ghost commented Jun 5, 2014

scrutinizer-notifier commented Jun 5, 2014

ghost commented Jun 5, 2014

scrutinizer-notifier commented Jun 5, 2014

DeepDiver1975 commented Jun 5, 2014

jancborchardt commented Jun 5, 2014