Skip to content

Navigation Menu

Sign in
Sign up

add icons to navigation of personal & admin settings - #3151

Merged
MorrisJobke merged 11 commits into
master from
navigation-icons
Jan 24, 2017
Merged

add icons to navigation of personal & admin settings #3151
MorrisJobke merged 11 commits into
master from
navigation-icons

Conversation

@jancborchardt

@jancborchardt jancborchardt commented Jan 19, 2017

Copy link
Copy Markdown
Member

Before & after:
capture du 2017年01月19日 02-27-02 capture du 2017年01月19日 02-26-35

capture du 2017年01月19日 03-11-08 capture du 2017年01月19日 03-10-45

This brings it in line with the other apps like Files, Activity, Mail, etc. :)
Please review @nextcloud/designers

A possible future improvement would be to instead of hardcoding the icons via the settings.css to get them from the app icon itself. cc @schiessle @nickvergessen @juliushaertl do you have an idea about the feasibility of that?

eppfel, juliusknorr, Espina2, and skjnldsv reacted with thumbs up emoji

Copy link
Copy Markdown
Member

Looks really nice, but the serverinfo app is missing an icon:

bildschirmfoto 2017年01月18日 um 21 48 15

@MorrisJobke MorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks nice, but we maybe should add the entry for the serverinfo app. I also added the viewBox elements where they were missing. 😉

Copy link
Copy Markdown
Member

I like it a lot too :)

Comment thread settings/css/settings.css Outdated
.nav-icon-workflow {
background-image: url('../img/tag.svg?v=1');
}
.nav-icon-survey_client {

@nickvergessen nickvergessen Jan 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is cheating and should be part of the api instead, otherwise apps that add their own sections dont have one....
I can do that...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably be in icons.scss.

@jancborchardt jancborchardt Jan 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@nickvergessen yup, that's why I mentioned it would be nicer to get it from the app! :) Would be cool if you can do that.
I looked in the activity app also but there you din't use the standard way with the ul class="with-icon" and instead of background-image use img elements. If possible we should use the standard as styled in the apps.css.

jancborchardt and others added 6 commits January 19, 2017 10:42 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>

Copy link
Copy Markdown
Member

I fixed the admin section, so the icons are now part of the API.
However I'm still not happy with the hack on the personal page, because apps can not get their icons displayed there without making changes in the server repo. But I guess we have to live with that until the personal page is rebuild with the new settings api as well.

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 19, 2017

Copy link
Copy Markdown
Member

Setting to develop because test will fail

 

Signed-off-by: Joas Schilling <coding@schilljs.com>
Comment thread settings/css/settings.css
margin-bottom: -3px;
margin-right: 6px;
width: 16px;
}

@jancborchardt jancborchardt Jan 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is what I mean – it doesn't use the same way as we do in Files, or Mail etc.

We should standardize that everywhere. Also cc @skjnldsv @eppfel regarding the design / HTML/CSS guidelines we want to improve.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I actually surrendered on nextcloud/nextcloud.com#140 for now. We need to find a more engaging, agile way to work on this. Let's talk @ FOSDEM about that...

Copy link
Copy Markdown
Member Author

@nickvergessen awesome! Just one thing: We should use the same method as we specified in the apps.css, which is the one we use in Files and Mail etc. Now there’s a discrepancy in method, and this even results in the spacing being different from the one in Files.

Copy link
Copy Markdown
Member

@jancborchardt Well activity uses image-paths as well and we need that when we want to make use of them in android/desktop apps as well.
Also images work without the android app needing to know all nextcloud apps, since its just a path you follow.

nickvergessen and others added 2 commits January 19, 2017 15:30 
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 19, 2017

Copy link
Copy Markdown
Member

Drone stalled on 5.6 again. I restarted the tests. 🙈 🙉 🙊

MorrisJobke merged commit e09bba5 into master Jan 24, 2017
MorrisJobke deleted the navigation-icons branch January 24, 2017 16:56

Copy link
Copy Markdown
Member

👍

MorrisJobke added a commit that referenced this pull request Jan 24, 2017 
* followup to #3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/serverinfo that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/survey_client that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/logreader that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/user_saml that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/files_accesscontrol that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/richdocuments that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit to nextcloud/twofactor_u2f that referenced this pull request Jan 24, 2017 
* follow up to nextcloud/server#3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke added a commit that referenced this pull request Jan 24, 2017 
* follow up to #3151
Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Copy link
Copy Markdown
Member

Looks really nice now:
bildschirmfoto 2017年01月24日 um 13 08 46

Espina2, hanzei, and skjnldsv reacted with thumbs up emoji

Copy link
Copy Markdown
Member Author

Yup, damn nice and consistent :) good work all around!

Now the only place missing icons is Help :)
skjnldsv, Espina2, and hanzei reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eppfel eppfel eppfel left review comments
@skjnldsv skjnldsv skjnldsv left review comments
@nickvergessen nickvergessen nickvergessen approved these changes
@MorrisJobke MorrisJobke MorrisJobke approved these changes

Labels

4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: settings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /