App navigation flex by skjnldsv · Pull Request #6399 · nextcloud/server

skjnldsv · 2017-09-07T08:21:25Z

Flex on app-navigation to finish the reorganisation of the app-navigation done on february by me.
Still some old code to clean.

codecov · 2017-09-07T12:47:23Z

Codecov Report

Merging #6399 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@ Coverage Diff @@## master #6399 +/- ##
============================================
+ Coverage 53.06% 53.06% +<.01% - Complexity 22552 22553 +1 
============================================
Files 1414 1414 Lines 87741 87745 +4 Branches 1340 1340 ============================================
+ Hits 46557 46564 +7 + Misses 41184 41181 -3

Impacted Files	Coverage Δ	Complexity Δ
apps/files/templates/appnavigation.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/files_trashbin/appinfo/app.php	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/NavigationManager.php	`79.38% <100%> (+0.31%)`	`43 <0> (+1)`	⬆️
apps/files_trashbin/lib/Expiration.php	`90.32% <0%> (-1.62%)`	`29% <0%> (ø)`
core/js/js.js	`61.83% <0%> (+0.55%)`	`0% <0%> (ø)`	⬇️

nickvergessen · 2017-09-08T14:25:19Z

I guess this will break a lot of apps again...

skjnldsv · 2017-09-08T14:28:34Z

@nickvergessen I will try my best to help reestablishing everything again. But would you prefer we do nothing and leave this mess around? 😞

And btw, there is a lot of feature announced on the css guidelines that don't work even before the scss switch. So it's time to have something usable again!

nickvergessen · 2017-09-08T14:30:05Z

No, of course progress is good. But maybe you can post in the forum in the dev section that there changes about this incoming, so people can check their apps and update them when necessary ;)

skjnldsv · 2017-09-08T14:32:21Z

I will check other apps myself.
I already have @ChristophWurst's request to do the mail menu, I will pr all the needed others :)

MorrisJobke · 2017-09-08T18:39:11Z

One thing that annoys me from time to time, is that the app content container is the actual scrolling part and the navigation around it is fixed. That also means, that mobile browsers don't properly detect if you scroll in there as a "page scroll" and don't hide the header of the browser. That means that it is unusable screen space. Does this PR here addresses this or does it only change stuff in the header/menu itself?

skjnldsv · 2017-09-08T18:42:22Z

@MorrisJobke not in this pr. But it's indeed an issue I'd like very much to investigate. Is there an opened one already?

MorrisJobke · 2017-09-08T18:47:12Z

Is there an opened one already?

I don't think so :/

skjnldsv · 2017-09-11T19:02:58Z

For testing, here is a test app which show all of the possibilities for the app-navigation.
templatetest.tar.gz

skjnldsv · 2017-09-12T03:47:32Z

@nextcloud/designers Please review!
I introduced some new stuff, based on the current guidelines.
Please take a look at the template test app which contains all possibilities and combination for the menu.
Tell me what you think!

skjnldsv · 2017-09-12T03:54:21Z

We should probably add a feedback on the submenus of a deletable entry. Right now we only get the confirmation on the main a content. We could set an opacity to 0.5? or something?

juliusknorr · 2017-09-12T08:02:08Z

@skjnldsv Looks really nice. One small issue I found while testing is that collapsed menus are only working when you click on the text. I think they should also collapse/expand when clicking the arrow symbol:

jancborchardt · 2017-09-12T10:22:09Z

One small issue I found while testing is that collapsed menus are only working when you click on the text. I think they should also collapse/expand when clicking the arrow symbol:

Every entry should be clickable on the whole height/width (extending to the left edge of the viewport, and to the right to the edge of the navigation, minus the action icons of course).

skjnldsv · 2017-09-12T12:55:50Z

@juliushaertl It's because of my javascript! ^^
I made it as simple as I could, becuse i didn't wanted to bother too much.

@jancborchardt there isn't any js in core to handle the app-navigation doesn't it?

jancborchardt · 2017-09-12T13:36:56Z

@skjnldsv not sure about the JS part but the clickable area is a pure CSS thing, no? Any idea ^ @BernhardPosselt @nextcloud/javascript?

skjnldsv · 2017-09-12T13:41:18Z

The clickable area is yes. I need to fix a small think indeed so the link gets above the collapse button.
We could also get rid of that button entirely and add it css only, it could be usefull since it will simplify what the user needs to set-up for the menu! :)

skjnldsv · 2017-09-21T16:52:52Z

🎉
Thank you everyone! Next stop: cleanup and documentation!! 🚋🚋🚋

pixelipo · 2017-09-21T17:10:38Z

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>


Great work with this PR, @skjnldsv ! Just a small note - SVG should be optimized with Scour and it's missing viewbox="0 0 16 16" tag.

I used the scour command provided by the nextcloud optimise bash script :p

Hmmm, that's strange, because it should remove the <xml> element as well as linebreaks....

scour --create-groups --enable-id-stripping --enable-comment-stripping --shorten-ids --remove-metadata apps/files/img/quota.svg > apps/files/img/quota-opt.svg

That's old, @skjnldsv - you missed #5423 😉

Damn it!!! 😅

pixelipo · 2017-09-21T17:12:30Z

 	display: none;
 }

-/* move Deleted Files to bottom of sidebar */


only 3 declarations left in this file - can we move them somewhere else?

skjnldsv · 2017-09-23T14:41:19Z

@MorrisJobke @jancborchardt Should we backport?
I think we should. We can't have apps supporting only 13+, 12 should be good at least.

MorrisJobke · 2017-09-24T19:37:26Z

As it is a feature I would say no - and for my opinion it is too much changed code for a back port

skjnldsv · 2017-09-25T03:15:00Z

Damn, I should have done that earlier.
Should I publish a simple patch to fit the old guidelines and fix the potentials flaws of a migrated app? For retro compatibility?

MorrisJobke · 2017-09-25T06:36:11Z

May be better - we only need to be careful about potential breakages of apps in the existing versions

jancborchardt · 2017-09-25T14:33:17Z

Yeah, this can absolutely not be backported unfortunately. :\ But yes, a drop-in CSS file for apps while they need to support older versions still might be best.

skjnldsv · 2017-09-25T14:34:38Z

Okay, I will provide a patch after I finished upgrading the other apps. When is 13 due?

MorrisJobke · 2017-09-25T16:27:07Z

When is 13 due?

~ 1 month feature freeze
~ 2 months release

skjnldsv · 2017-09-25T16:28:53Z

Meep meep! 📯
Time to speed up @skjnldsv ^^

skjnldsv added 2. developing Work in progress design Design, UI, UX, etc. medium labels Sep 7, 2017

skjnldsv self-assigned this Sep 7, 2017

skjnldsv force-pushed the app-navigation-flex branch from ee4d48e to 0e81b5cCompare September 7, 2017 08:50

skjnldsv force-pushed the app-navigation-flex branch from 043d015 to fabe262Compare September 10, 2017 14:17

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 12, 2017

skjnldsv requested review from ChristophWurst, Henni, MorrisJobke, jancborchardt, juliusknorr and nickvergessen September 12, 2017 03:46

skjnldsv mentioned this pull request Sep 21, 2017
Fit app-navigation to css guidelines nextcloud/mail#521
Closed

skjnldsv merged commit e336283 into masterSep 21, 2017

skjnldsv deleted the app-navigation-flex branch September 21, 2017 16:50

pixelipo reviewed Sep 21, 2017
View reviewed changes

jancborchardt mentioned this pull request Sep 22, 2017
App navigation styles: entries in folders not highlighted correctly, incorrect padding for entries with counters #4629
Closed

juliusknorr mentioned this pull request Sep 23, 2017
Settings menu layout is broken #6622
Closed

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. medium and removed 3. to review Waiting for reviews design Design, UI, UX, etc. medium labels Sep 23, 2017

This was referenced Sep 23, 2017
App to css guidelines nextcloud/news#220
Merged
Move to SCSS [was: App to css guidelines] nextcloud/calendar#613
Merged
Fix app nav img margin #6623
Merged

skjnldsv added the backport-request label Sep 23, 2017

skjnldsv removed the backport-request label Sep 25, 2017

juliusknorr mentioned this pull request Oct 3, 2017
App navigation is broken on NC 13 nextcloud/deck#315
Closed
2 tasks

skjnldsv added the standardization label Feb 15, 2018

skjnldsv mentioned this pull request Feb 15, 2018
Standardisation summary #8374
Closed

Uh oh!

Uh oh!

Conversation

skjnldsv commented Sep 7, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nickvergessen commented Sep 8, 2017

Uh oh!

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

Uh oh!

Uh oh!

nickvergessen commented Sep 8, 2017

Uh oh!

skjnldsv commented Sep 8, 2017

Uh oh!

MorrisJobke commented Sep 8, 2017

Uh oh!

skjnldsv commented Sep 8, 2017

Uh oh!

MorrisJobke commented Sep 8, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

skjnldsv commented Sep 12, 2017

Uh oh!

juliusknorr commented Sep 12, 2017

Uh oh!

jancborchardt commented Sep 12, 2017

Uh oh!

skjnldsv commented Sep 12, 2017

Uh oh!

jancborchardt commented Sep 12, 2017

Uh oh!

skjnldsv commented Sep 12, 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!

pixelipoSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsvSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

pixelipoSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsvSep 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!

pixelipoSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsvSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

pixelipoSep 21, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

MorrisJobke commented Sep 24, 2017

Uh oh!

skjnldsv commented Sep 25, 2017

Uh oh!

MorrisJobke commented Sep 25, 2017

Uh oh!

jancborchardt commented Sep 25, 2017

Uh oh!

skjnldsv commented Sep 25, 2017

Uh oh!

codecovBot commented Sep 7, 2017•
edited
Loading

skjnldsv commented Sep 8, 2017•
edited
Loading

skjnldsv commented Sep 11, 2017•
edited
Loading

skjnldsv commented Sep 12, 2017•
edited
Loading

skjnldsv commented Sep 21, 2017•
edited
Loading

skjnldsvSep 21, 2017•
edited
Loading

skjnldsv commented Sep 23, 2017•
edited
Loading