-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consistent icon behavior #5048
Comments
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 |
My personal opinion on this topic: 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. |
icon classes are deprecated since NC20.
No and no. Icon URLs are also very problematic for dark mode I would be ok to keep those, but we need to make sure it's dark mode compatible. |
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. |
Well, we've had that icon API discussion thousands of times in the past. 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. TL;DR, let's also move towards dropping URLs too ? 👉👈 |
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. |
Thanks for details explanation @skjnldsv 💙 |
@susnux you can pass svg as string straight away from the php API |
@skjnldsv but that would increase the api response quite a lot. Especially because it can not be cached. |
@susnux which is alright, like said above:
We can try to measure that, but I do not think this would be such a crazy difference. |
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. |
Btw for those discussions we usually have https://github.com/nextcloud/standards/issues for this. |
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:
And from that questions we should probably make all components consistent with the icon prop.
cc @skjnldsv @raimund-schluessler @ShGKme for input 😃
The text was updated successfully, but these errors were encountered: