Skip to content
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

[Sharing NG] Add remix icon to share roles #8844

Closed
JammingBen opened this issue Apr 12, 2024 · 7 comments
Closed

[Sharing NG] Add remix icon to share roles #8844

JammingBen opened this issue Apr 12, 2024 · 7 comments
Assignees
Labels
Category:Enhancement Add new functionality

Comments

@JammingBen
Copy link
Contributor

JammingBen commented Apr 12, 2024

Description

Add icons for roles in the themes.json in the common section.

We add key>value pairs of name->path/to/icon.svg

The icons are basically strings representing an icon from https://remixicon.com/.

  • All viewer roles: eye
  • All editor roles: pencil
  • Space manager role: user-star
  • Secure view role: shield-fill

cc @micbar @rhafer

@JammingBen JammingBen added the Category:Enhancement Add new functionality label Apr 12, 2024
@JammingBen JammingBen mentioned this issue Apr 12, 2024
40 tasks
@tbsbdr tbsbdr moved this from Qualification to Backlog in Infinite Scale Team Board Apr 12, 2024
@micbar micbar self-assigned this Apr 12, 2024
@fschade fschade assigned fschade and unassigned micbar Apr 17, 2024
@micbar micbar moved this from Backlog to In progress in Infinite Scale Team Board Apr 17, 2024
@fschade
Copy link
Contributor

fschade commented Apr 19, 2024

Done statically in the theme json, no server is involved

@fschade fschade closed this as completed Apr 19, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Infinite Scale Team Board Apr 19, 2024
@kulmann
Copy link
Member

kulmann commented Apr 20, 2024

Done statically in the theme json, no server is involved

Why did you go with the theme.json instead of returning the iconnames from the server? Please document the reasoning here.

Solution seems to be suboptimal to me. We have customers with a custom theme. They will need to adapt thir theme with the next release, which doesn't feel right.

@kulmann kulmann reopened this Apr 20, 2024
@github-project-automation github-project-automation bot moved this from Done to In progress in Infinite Scale Team Board Apr 20, 2024
@fschade
Copy link
Contributor

fschade commented Apr 20, 2024

since the icon is something visually we’ve decided to ship that via the theme.json.

Currently a manual change is needed, we also discussed if the server should take care of injecting the icons there, for the moment we’ve decided to not do that. But I’m absolutely fine implementing such a thing (theme pre processing).

Let me know what you think, even if we decide to have something else, in my opinion it’s great to give the user the ability to have it in the theme since it’s for me the single source of truth for everything that’s visually

@kulmann
Copy link
Member

kulmann commented Apr 21, 2024

As long as we don't support exchanging the entire icon set (I don't see that coming!) it's fine for the server to return the icon names. That's by far the simplest solution I can think of. Having a path in the theme even contradicts the way web currently loads its icons (which is.... by name, not by path).

I don't have a current status on the cross client compatibility (i.e. are the remixicons available in all clients). IMO that should be the case but I doubt that we followed through with it.

What you describe sounds like a cool thing to have in the future (theme preprocessing makes a lot of sense) but theming is not our current product focus so for what we currently have it sounds overengineered. 🙈

@fschade
Copy link
Contributor

fschade commented Apr 21, 2024

Let’s revisit that topic tomorrow after the standup, I’m sure we find a way to make it wow 🤩

@kulmann
Copy link
Member

kulmann commented Apr 22, 2024

Please note: owncloud/web#10801 (comment)

@fschade
Copy link
Contributor

fschade commented Apr 24, 2024

we decided to add a follow up issue which adds a theme processor and ensures defaults.
closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
Archived in project
Development

No branches or pull requests

4 participants