nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.71k stars 4k forks source link

Better handling of inverted icons #5593

Closed pixelipo closed 6 years ago

pixelipo commented 7 years ago

My thoughts related to #5450 - which is just one example of problems with icons in NC.

I don't think using icon-more-white to display dark variant of the icon is a very elegant approach. I'm wondering if we can come up with a better solution for such situations ( #5450 is only an example). Here are several proposals that we might want to consider in future releases:

  1. SVG: use inline SVG as icon source (many pros and cons, I'm not going to list them here), set fill in CSS as needed
  2. JS: toggle between icon-more and icon-more-white classes using JS
  3. CSS: use CSS filter: invert(100%) property, which is quite nicely supported already

My personal favourite of those is fill, but filter() would also be neat. Both of these would allow us to deprecate a considerable amount of existing icons. They would also give us possibility to theme icons (I guess folder icon is the only obvious candidate here). SVG-based solution would additionally allow us to have icons-within-icons without maintaining a separate asset, such as core/img/filetypes/folder-public.svg - but it might not be feasible just yet :cry:

I would like to hear your thoughts @nextcloud/designers @nextcloud/theming

jancborchardt commented 7 years ago

Important is that we keep supporting the browsers we need to support.

I agree with the methods you prefer @pixelipo. Maybe @juliushaertl @nextcloud/designers @nextcloud/javascript have more input?

(Btw @pixelipo this is not a papercut, which is a user-facing small issue - it rather is technical debt. :)

juliushaertl commented 7 years ago

Important is that we keep supporting the browsers we need to support.

Is there a note somewhere what browsers are currently supported? Cannot find it atm.

CSS: use CSS filter: invert(100%) property, which is quite nicely supported already

We already use this for the theming app, so i see no problem there. Seems to be the best solution to me.

MorrisJobke commented 7 years ago

Is there a note somewhere what browsers are currently supported? Cannot find it atm.

IE11+ and the current versions of the others usually (+ the LTS versions of those browsers).

pixelipo commented 7 years ago

IE11 is at ~3.3% global use. In that case, this idea should be put on hold until Edge takes over IE11 or figure out some smart IE fallback

Espina2 commented 7 years ago

The first one are the better solution, less http requests, no longer need to be pure black ou pure white, we can change it to wherever they want.

MorrisJobke commented 7 years ago

IE11 is at ~3.3% global use. In that case, this idea should be put on hold until Edge takes over IE11 or figure out some smart IE fallback

Usually IE11 is not that hard to convince. But I guess it's not an option to drop it, because there are some customers demanding it AFAIK. cc @karlitschek

karlitschek commented 7 years ago

Yes. We need to wait a bit longer until we can drop it. Unfortunately

pixelipo commented 7 years ago

That's understandable, @karlitschek @MorrisJobke

Looking at this repo's README.md, it looks like we are not likely to get CSS filter() working on IE11, even with using a polyfill. I didn't Google too much.

filter() is a much quicker solution to implement, but it's off because of IE11.

SVG is overall a better solution, works in all browsers and - I think - something likely to become a 'default' on the web in a couple of years, but it will require more work. I will try to create a "proof of concept" PR and we can see if it will work for NC.

pixelipo commented 7 years ago

@juliushaertl you're referring to this code in theming.scss, right?

#appmenu li a img {
    -webkit-filter: invert(1);
    filter: invert(1);
    filter: progid:DXImageTransform.Microsoft.BasicImage(invert='1');
}

Did anyone check whether this even works in IE11? From what I managed to Google, such a solution is only valid for IE<10 I tried a quick'n'dirty test and it doesn't work...

pixelipo commented 7 years ago

Here it is, #5682

pixelipo commented 7 years ago

Copying my comment from #5682 over here for reference:

@MorrisJobke not that easy, in this PR I'm still using icon-contacts css - but only because of [class^='icon-'] declarations.

I think I will just create a new folder core/svg/, populate it with inline-svg versions of icons and create a new set of css rules like this [class^='svg-']. That will make sure we stay backwards compatible but allow any app or page to convert to the inline-svg solution.

I'm also considering the idea of creating a new PHP method (and maybe a compatible JS function as well) for fetching/injecting SVG by class(file)name. Is there any existing PHP class that could host such a method?

P.S. @jancborchardt would you consider this to be a duplicate of #1066 ? They are about the same issue, but propose different solutions.

jancborchardt commented 7 years ago

@pixelipo I’d keep it separate because regardless if the specific solution works or not, the goal (improving performance by minimizing HTTP requests) is still valid.