Consistent icon behavior

[susnux]

We have several places where we allow to use a prop for the icon, but this prop is not consistent across the components.
So I would like to clarify this for the upcoming version 9:

1. Do we still want to support icon classes using the icon prop
2. Do we still want to support URLs in the icon prop
3. Do we want to keep the icon prop at all?
4. If we keep it, do we want to support SVGs passed to the icon prop?

And from that questions we should probably make all components consistent with the icon prop.

cc @skjnldsv @raimund-schluessler @ShGKme for input 😃

[ShGKme]

I think, we should keep it. And I'd even return the icon prop to some components like NcButton.

We have some JS API, where an icon class name is provided, and some HTTP API that provides URLs. For example, Files, contact menu.

Developers still can provide an icon to the icon slot, but in this case it is more complex to control that it is defined correctly (e.g. a11y attrs, invert on dark) and to keep consistentcy.

If we decide to keep only the icon slot, then I'd first try to migrate all the apps.

I'm not sure we need the icon prop for SVG (do we use it anywhere?). But iconPath could be useful as done in NcIconAvgWrapper, IMO

[susnux]

My personal opinion on this topic:
URLs are returned by our core API so we need this we should support relative and absolut URLs, otherwise all users would have to use icon slot with custom handling.

But I think we do not need the icon classes anymore but should encourage all to use SVGs or SVG paths.

So I am not sure if we should support icon prop for URLs and SVG-paths or just URLs and add an icon-path prop.
I am a bit in favor of only one prop as URLs are somewhat easy to distinguish from SVG paths.

[skjnldsv]

• Do we still want to support icon classes using the icon prop

icon classes are deprecated since NC20.
At one point it will be removed entirely ™

• Do we still want to support URLs in the icon prop
• Do we want to keep the icon prop at all?

No and no. Icon URLs are also very problematic for dark mode
The only reason to have urls is for php APIs.

I would be ok to keep those, but we need to make sure it's dark mode compatible.

[ShGKme]

The only reason to have urls is for php APIs.

And we are not going to get rid of using URLs in HTTP API, right? I guess, rendering pure SVG may not be pleasant for apps, like native notifications for example.

[skjnldsv]

And we are not going to get rid of using URLs in HTTP API, right?

Well, we've had that icon API discussion thousands of times in the past.
Now that svg can be rendered inline in most places, it helps tremendously.
The only issue about using inline svg in APIs is that you cannot cache it add it can use a fair amount of extra bandwith if used too much.

We thought about having a dedicated component fetching URLs and caching, we thought about having a library of icons and using a reference only... plenty of ideas...

In the end, and it's my two cents, I think the inline svg covers all of our needs.
Flexible, can be imported from an svg file from a php API, is dark-mode compatible.
While it could indeed, theoretically, add a bit more bandwidth to our page loading, this would be async and I think our php APIs are not used that often to actually have such a big impact.

TL;DR, let's also move towards dropping URLs too ? 👉👈

[susnux]

The places where I still found images are APIs where other apps might register responses (like activites etc) and your frontend code does not know which SVG to use.
I am not sure how this could be handled, but as non of that APIs is deprecated we will have to handle URLs for at least 3+ years.

[ShGKme]

Thanks for details explanation @skjnldsv 💙

[skjnldsv]

@susnux you can pass svg as string straight away from the php API
Especially with activity, I think that would work well 👀

[susnux]

can pass svg as string

@skjnldsv but that would increase the api response quite a lot. Especially because it can not be cached.

[skjnldsv]

@susnux which is alright, like said above:

While it could indeed, theoretically, add a bit more bandwidth to our page loading, this would be async and I think our php APIs are not used that often to actually have such a big impact.

We can try to measure that, but I do not think this would be such a crazy difference.

[ShGKme]

But shouldn't we first get rid of icon-classes and URLs in JS/HTTP API and then remove such props from the components?

Otherwise, we don't control if the icon is passed correctly in meaning of a11y and the dark mode.

[skjnldsv]

Btw for those discussions we usually have https://github.com/nextcloud/standards/issues for this.
Create a discussion, take a vote, decide and move from there