Use darker colors for app icons in app management. #834
Conversation
|
@raghunayyar, thanks for your PR! By analyzing the annotation information on this pull request, we identified @DeepDiver1975, @jancborchardt and @MorrisJobke to be potential reviewers |
raghunayyar
force-pushed
the
feature/similar-app-colors
branch
from
9a6b486 to
bc74239
Compare
raghunayyar
added
enhancement
design
|
@raghunayyar Thank you very much for your contribution. I think simply replacing the colors inside the SVGs will not work, as this would mean we need to do so in every app (also 3rd party ones). That means we'll lose compatibility to existing OwnCloud apps. See #649 (comment) @nextcloud/designers @jancborchardt what do you think? |
|
Personally, I would prefer to take a hybrid approach. For example, elsewhere it was previously suggested that we add an entry to the app info, specifically to point to a nextcloud specific app icon. OwnCloud simply wouldn't recognize this and would skip it. So we could look for a (dark) Nextcloud icon being set, and then if we didn't find it, fall back to taking the ownCloud one and run an SVG matrix filter over it like: ...to invert it. The only thing I'm not sure about is if there is sufficient cross browser support. |
raghunayyar
force-pushed
the
feature/similar-app-colors
branch
from
bc74239 to
c489cd4
Compare
|
I have updated the PR and re-implemented it with svg filters, though I am not liking it visually. Black on white (100% inversion) is creating way too much contrast. I would like @jancborchardt / @nextcloud/designers to take the call. Here is a screenshot: |
Then simply add a opacity of 60% to it via CSS 🙈 😉 |
|
|
|
@MorrisJobke done. All yours. ;) |
|
@raghunayyar Looks good so far, just one more thing: For app list coming from the app store, there is a preview of the apps shown. We need to make sure, that this previews don't get inverted like the icons.
|
raghunayyar
added
2. developing
|
Great progress here! Looks good except for the issue @juliushaertl already noted. :) |
|
Will fix this one today. |
|
Fixed, can we review this. :) |
raghunayyar
added
3. to review
|
looks good now 👍 cc @nextcloud/designers for another reviewer |
|
👍 |
|
We want to have this in |
|
nice. please backport |
|
BTW Congrats on your first PR @raghunayyar ! |
|
I will create the backport PR :)
Yes! 🎉 |
|
Whoops... Forgotten to backport 😁 Sorry and THX @MorrisJobke 😅 |
| img += '<image x="0" y="0" width="72" height="72" preserveAspectRatio="xMinYMin meet" xlink:href="' + url + '" class="app-icon" /></svg>'; | ||
| } else { | ||
| img += '<defs><filter id="invert"><feColorMatrix in="SourceGraphic" type="matrix" values="-1 0 0 0 1 0 -1 0 0 1 0 0 -1 0 1 0 0 0 1 0" /></filter></defs>'; | ||
| img += '<image x="0" y="0" width="72" height="72" preserveAspectRatio="xMinYMin meet" filter="url(#invert)" xlink:href="' + url + '" class="app-icon" /></svg>'; |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/image cough cough ... this breaks the app managment in all browsers except chrome (where it only works by luck). I will revert it and we better load the SVG directly into the DOM which is even supported in IE9 and makes CSS styling easier too.
There was a problem hiding this comment.
Ah just noticed that this is wrapped inside an SVG ... then I will try to fix this.
There was a problem hiding this comment.
|
Regression: #900 |
…olors Use darker colors for app icons in app management.
Should ideally fix #649, but the icons are coming as white for me, though I have changed the fills of all the icons. Can someone cross check if it is working fine for them?
@jancborchardt I am not using #000 but #404040 which is the exact colour when you hover the icons in the app navigation. #000 cause a lot of contrast. :-)
Btw this is my first PR to NC ;-)