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

[Experimental] Add support for using components as icons #481

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

piemonkey
Copy link
Contributor

@piemonkey piemonkey commented Mar 13, 2024

Warning

This setup is still in an experimental state. We want to try it out in a couple of specific places before considering this ready to use by all the Appuniversum users. We might still break the setup in future minor versions until we announce it as stable.


Another PR in the line of PRs attempting to address the issue of inline SVGs.

This time the approach is to allow passing of arbitrary components to AuIcon (and by extension, any component taking an @icon argument).

The advantages of this method are that this is very flexible to the user. Also, by overloading @icon it avoids confusion that could be caused by adding a new argument, as only one of string or Component can be passed.

A minor downside is that the user can potentially pass a component that could cause rendering issues.

A risk is that in order to pass arguments to the component, for example; to pass an AuIcon as a custom component, you need an argument to specify which icon to use; you must use the component helper. In this way it is easy to write code that embroider can't statically analyse.

Copy link
Contributor

@Windvis Windvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Some quick feedback. I'll try to take a better look tomorrow.

@piemonkey piemonkey force-pushed the feature/pass-icon-component branch from 1679623 to ccdebfa Compare March 14, 2024 09:30
@Windvis
Copy link
Contributor

Windvis commented Mar 14, 2024

Thinking about this some more. I don't think this will fix the problem in the frontend-embeddable-notule-editor project, since we also use the AuIcon component internally with some "hardcoded icons" and those would still look at the symbolset version. Making those configurable would be strange, since the icon is just an implementation detail of the component.

So while I still think accepting component for @icon is a good improvement (to allow custom icons) it still won't fix the problem you are seeing in the project. 🤔

@Windvis Windvis mentioned this pull request Mar 14, 2024
6 tasks
@piemonkey piemonkey force-pushed the feature/pass-icon-component branch from ccdebfa to e6413b5 Compare March 19, 2024 14:20
@piemonkey piemonkey force-pushed the feature/pass-icon-component branch from e6413b5 to 5780b3d Compare March 20, 2024 16:31
@Windvis Windvis changed the title Modify AuIcon to take an inline component for the icon [Experimental feature] Add support for using components as icons Mar 20, 2024
@Windvis Windvis added the enhancement Used when the PR adds a new feature or enhancement. label Mar 20, 2024
@Windvis Windvis merged commit c60e976 into appuniversum:master Mar 20, 2024
8 checks passed
@Windvis Windvis changed the title [Experimental feature] Add support for using components as icons [Experimental] Add support for using components as icons Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Used when the PR adds a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants