appuniversum / ember-appuniversum

Ember addon wrapping the appuniversum components.
https://appuniversum.github.io/ember-appuniversum
MIT License
14 stars 11 forks source link

Appuniversum icon improvements #482

Open Windvis opened 5 months ago

Windvis commented 5 months ago

alternative to the symbolset setup [Released as experimental]

Read more At the moment Appuniversum bundles a [list of icons](https://github.com/appuniversum/ember-appuniversum/tree/654711add804e8e2de2a5b23b22976690a9c4b90/public/icons) into a svg-symbols file which is also stored in the public folder and which is then referenced in `AuIcon` component. This works, but has some downsides: - no support for custom icons, which means icons need to be added here, even if they are very specific for a single project - All the icons are bundled, even if only a few are used by a project - If the app is embedded in a different domain, the browser might refuse to load the referenced symbolset, so something like [svgxuse](https://github.com/Keyamoon/svgxuse) needs to be used as a workaround. A potential solution for these issues is to use icon components, instead of the string based setup we have now. Each .svg file could be converted to a .gts file at build time, and imported like any other component. The benefits: - Since they're components, Embroidered apps will strip out the unused ones so only the used icons will be part of the build - No reference to a symbolset file, so no issues with cross-domain requests For this to work, each component that now accepts a string for the `@icon` argument, would also need to support a component (POC in #481 ). We would also need to refactor our internal usage of the `AuIcon` component, and pass the components instead of the string. TODO: - [x] Add support for passing components into the `@icon` arguments (#481) - [x] Add a script that generates icon components from the .svg files (#487) - [x] Use the icon components internally (#486) - [x] Release this work as an experimental feature, so apps can start experimenting with it (Released in [v3.4.0](https://github.com/appuniversum/ember-appuniversum/blob/master/CHANGELOG.md#v340-2024-03-25)) **Open questions:** - Do we want to deprecate the string based system? If we don't any usage of the symbolset will trigger the download resulting in all the icons being downloaded again) - Do we want to expose these components in Ember's global component namespace, or do we only expose the export paths and require that consumers import where needed? (In non-gjs/gts files this might be cumbersome, but I think we're at a point where using .gjs is in a good place)

Webuniversum icon inconsistencies

Webuniversum has a lot of icons. Not all of them were added here, and we might also be using some of the legacy ones.

We should compare all the icons we bundle here with the Webuniversum list and document if it is a custom component, or a legacy one.

TODO:

Custom icons

If an icon is not part of the Webuniversum list, we should check were it is being used. If it isn't used, or if a Webuniversum equivalent with a different name exists, we should deprecate it (or otherwise override it).

Legacy icons

We should deprecate the legacy icons and suggest the new equivalent.

Sync all the icons

We should also add all the icons that Webuniversum currently has so they can easily be used without new PRs. Assuming we only support the new icons in the component setup this shouldn't have an impact.

piemonkey commented 5 months ago

If the app is embedded in a different domain, the browser might refuse to load the referenced symbolset, so something like svgxuse needs to be used as a workaround.

In fact, while the svgxuse workaround does work if you're hosting the ember app that is embedded in another domain (for example in an iframe), as svgxuse is still able to request the symbolset. If the app is designed to be bundled within another project, then the symbolset is no longer available and this approach fails. This is the case for the embeddable-say-editor project which can now be installed through npm and in order to work requires overriding AuIcon with a custom version. This workaround only works for ember-appuniversum <=2.15.

abeforgit commented 5 months ago

@Windvis do you have an idea of when this could land/how much work it would be? Just to get an idea for our planning. @piemonkey what is your intuition on how much work this would be to do ourselves?

Windvis commented 5 months ago

@abeforgit The first part could be done fairly quickly, if I can't get time for it this sprint I can probably get time for it in the next one. @piemonkey did the groundwork for the component based icons, and I have a WIP PR that generates icon components from the .svg files. Once both are merged we can use these components internally.

I think we should release this as an experimental feature that apps can opt-into though. I don't feel confident enough about this setup to consider it stable, and I might want to change some things which might be breaking, without having to cut a major just for that.

The icon inconsistencies are not super high prio, so that can be done later since there is quite a bit of grunt work involved.

So current plan:

The embeddable could then update to v3+ and test it out. I'll try to test it in one of the projects I work on as well. If all the kinks are resolved we can release it as a stable feature and deprecate the string setup.

Windvis commented 5 months ago

@piemonkey @abeforgit The icon component setup is released as an experimental feature in 3.4.0.

I don't foresee any huge breaking changes in the future. I think the API itself is fine. There might be some breaking changes once we sync the shipped icons with those from Webuniversum, but that will most likely be limited to the removal (in case of duplicates) / renaming of some icons, so should be easy to fix.

Windvis commented 5 months ago

First issue when trying some of the icon components:

Some icons have a fill="none" attribute which makes the icon invisible. For example the visible icon (the irony..).

The symbolset version doesn't have the same issue because the library we use strips out all the fill attributes (among others).

We should probably just fix the icons here, and maybe lint them in some way? I'm not the biggest fan of stripping them magically since it just hides the issue and there must be a reason why the attribute was added.

As a workaround you can inline the icon in a new component in your project, remove the fill and use that until the issue is resolved in the Appuniversum version.

This is something we have to keep in mind when we sync the new icons.

piemonkey commented 5 months ago

Ha, funny, I found one of these (link-broken) and was about to put in a PR with the fill removed from it.

I definitely think stripping the fill from the svg is the way to go. Not sure how best to go about linting them though.

Windvis commented 5 months ago

I definitely think stripping the fill from the svg is the way to go. Not sure how best to go about linting them though.

I think stripping it from the source is the way to go, but I don't like the 'let's do it during the build step' method, at least for now. I don't know enough about svg or why design programs output this attribute when exporting the svg to be sure we can simply strip it in all cases 😄. On the other hand, that's how we were already using the icons before, so it probably would be fine (for the icons we already have).

I still think updating the .svg is better though. Maybe we should just check the presence of fill="none" in the build script and throw an error?

Windvis commented 5 months ago

@piemonkey Should be fixed by https://github.com/appuniversum/ember-appuniversum/pull/488. It solved the issue I noticed at least 👍.