Skip to content

Theming: invert foreground color on bright backgrounds - #412

Merged
nickvergessen merged 4 commits into
masterfrom
theming-foreground-color
Jul 18, 2016
Merged

Theming: invert foreground color on bright backgrounds#412
nickvergessen merged 4 commits into
masterfrom
theming-foreground-color

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Jul 15, 2016

Copy link
Copy Markdown
Member

As discussed in #378, this pull request extends the theming app to invert the text/icon color inside
the header for bright colors.

Algorithm for luminance calculation: https://www.w3.org/TR/AERT#color-contrast

Before:
2016-07-14-211653_960x529_scrot
After:
2016-07-14-211612_960x529_scrot

By now the Nextcloud logo will not be touched, as specified by the Nextcloud Trademark Guidelines:

The logo is light blue with black text on white. It should not be used differently, especially not inverted. The name "Nextcloud" can be optionally left out though, we provide logo's without the text.

cc @nextcloud/designers @schiessle

@mention-bot

mention-bot commented Jul 15, 2016

Copy link
Copy Markdown

@juliushaertl, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @oparoz and @Henni to be potential reviewers

oparoz
oparoz reviewed Jul 15, 2016
View reviewed changes
Comment threadapps/theming/js/settings-admin.js Outdated
function calculateLuminance(rgb) {
var hexValue = rgb.replace(/[^0-9A-Fa-f]/,'');
var r,g,b;
if(hexValue.length === 3) {

@oparozoparozJul 15, 2016

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.

Missing space between if and (.
But it would be worth parsing the whole code so that all such errors are automatically fixed.

@oparoz

oparoz commented Jul 15, 2016

Copy link
Copy Markdown
Member

Looks good. The logo part can be taken care of in another PR, simply detect if a custom one is used and if there is a dark version.

I'm not a fan of static methods, but everything seems tested, so it's not a show stopper.

@juliusknorr

juliusknorr commented Jul 15, 2016

Copy link
Copy Markdown
MemberAuthor

All right, thanks. Spaces should be fixed now.

@jancborchardtjancborchardt added enhancement design Design, UI, UX, etc. labels Jul 15, 2016
@jancborchardt

jancborchardt commented Jul 15, 2016

Copy link
Copy Markdown
Member

Wow, awesome! :) Really good stuff @juliushaertl

@nextcloud/designers @Lord-Protector@schiessle let’s give this a good test! Also with the installation process etc.

@juliushaertl@oparoz the logo we will leave entirely untouched, yes. Because when you change the theming, you will see that a light logo will not work. That’s not something we should automatically modify.

@juliusknorrjuliusknorr mentioned this pull request Jul 15, 2016
Merged
2 tasks
@schiessle

schiessle commented Jul 15, 2016•
edited
Loading

Copy link
Copy Markdown
Member
  • If I change the color the arrow next to drop down menu disappears, otherwise great work @juliushaertl !

@schiessle

schiessle commented Jul 15, 2016

Copy link
Copy Markdown
Member

The JavaScript console shows this error:

http://localhost/core/img/actions/caret.svg 404 (Not Found)

the correct path would be http://localhost/master/core/img/actions/caret.svg

@oparoz

oparoz commented Jul 15, 2016

Copy link
Copy Markdown
Member

Good point @jancborchardt.

@savely-krasovsky

savely-krasovsky commented Jul 15, 2016•
edited
Loading

Copy link
Copy Markdown

@jancborchardt theoretically logo will have a good look with some shade of gray.
But it requires study.

@juliusknorr
juliusknorrforce-pushed the theming-foreground-color branch from 30447ca to d8bc52cCompareJuly 16, 2016 07:08
@juliusknorr
juliusknorrforce-pushed the theming-foreground-color branch from d8bc52c to 48ac845CompareJuly 16, 2016 07:19
@juliusknorr

juliusknorr commented Jul 16, 2016

Copy link
Copy Markdown
MemberAuthor

The disappearing icon should be fixed now.
@schiessle

@schiessle

schiessle commented Jul 18, 2016

Copy link
Copy Markdown
Member

everything works now... Great work! 👍

@nickvergessen

nickvergessen commented Jul 18, 2016

Copy link
Copy Markdown
Member

Works and looks good 👍

Regarding:

By now the Nextcloud logo will not be touched, as specified by the Nextcloud Trademark Guidelines:

The logo is light blue with black text on white. It should not be used differently, especially not inverted. The name "Nextcloud" can be optionally left out though, we provide logo's without the text.

Not sure about this, maybe @karlitschek can help, because in this case the logo was already recolored (it's white not light-blue)

@nickvergessen
nickvergessen merged commit 89a32a2 into masterJul 18, 2016
@nickvergessen
nickvergessen deleted the theming-foreground-color branch July 18, 2016 10:43
@nickvergessen

nickvergessen commented Jul 18, 2016

Copy link
Copy Markdown
Member

@juliushaertl want to make a PR against stable9 as well?

@nickvergessennickvergessen added this to the Nextcloud Next milestone Jul 18, 2016
@schiessle

schiessle commented Jul 18, 2016

Copy link
Copy Markdown
Member

Not sure if we should backport it... It is not a critical bug. I would prefer to keep it for the next release

@karlitschek

karlitschek commented Jul 18, 2016

Copy link
Copy Markdown
Member

True. @jospoortvliet We should update our trademark policy to also allow white on blue.

@jancborchardt

jancborchardt commented Jul 18, 2016

Copy link
Copy Markdown
Member

@karlitschek theming has nothing to do with the trademark policy though I would say. When theming your Nextcloud, you should also use a different logo. :)

@karlitschek

karlitschek commented Jul 18, 2016

Copy link
Copy Markdown
Member

true :-)

@nickvergessennickvergessen mentioned this pull request Jul 18, 2016
Closed
@juliusknorrjuliusknorr mentioned this pull request Aug 8, 2016
Closed
10 tasks
R0Wi pushed a commit to R0Wi/server that referenced this pull request Nov 22, 2025
@edward-ly@bigcat88
fix(AppAPIService): encode query parameter arrays via http_build_query (
f15c6ea
nextcloud#412)
Resolvesnextcloud#384. If the checks still pass and this gets merged, perhaps we
should take a look at other apps to see if they need to be updated in
the same way.
Signed-off-by: Edward Ly <contact@edward.ly>
Co-authored-by: Alexander Piskun <13381981+bigcat88@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsdesignDesign, UI, UX, etc.enhancementfeature: theming

Projects

None yet

Milestone

Nextcloud 10.0

Development

Successfully merging this pull request may close these issues.

8 participants

@juliusknorr@mention-bot@oparoz@jancborchardt@schiessle@savely-krasovsky@nickvergessen@karlitschek