appuniversum / ember-appuniversum

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

Use svg-jar for rendering svgs inside au-icon #424

Closed elpoelma closed 5 months ago

elpoelma commented 1 year ago

Overview

This PR introduces the usage of the ember-svg-jar package in order to render the svgs inside the au-icon component. The usage of ember-svg-jar provides the following advantages:

The consuming app can provide more configuration options described on https://github.com/ghedamat/ember-svg-jar/blob/master/packages/ember-svg-jar/docs/configuration.md

Possible issues

It is not possible to pass splattributes to the svg-jar helper provided by ember-svg-jar. This PR introduces an additional span element which accepts the splattributes around the svg-jar helper. However, when searching for the usage of the AuIcon component in the projects using ember-appuniversum no usage of these splattributes on au-icon is found.

nvdk commented 1 year ago

I think this also means you can drop the prepare action here: https://github.com/appuniversum/ember-appuniversum/blob/master/package.json#L35 and the svg-symbols package.

elpoelma commented 1 year ago

I think this also means you can drop the prepare action here: https://github.com/appuniversum/ember-appuniversum/blob/master/package.json#L35 and the svg-symbols package.

I'd think so yes, but is it not advantageous to keep the symbol file, in order to reduce the svg file size in comparison to seperate svg files?

Dietr commented 1 year ago

Seems to work but I see a few issues:

In general the inlining of separate svg's has a disadvantage over the use method. If you have a table where each row includes an icon you have to repeat the svg code multiple times. On long tables or multiple repetitions this will increase the file size quite a lot. Is inlining the svg symbolset not an option with svg-jar?

elpoelma commented 1 year ago

Seems to work but I see a few issues:

* introduction of the span breaks alignment in multiple components. I think we need the icon classes on the span element and if necessary adjust these classes so alignment is fixed. (see: https://www.chromatic.com/review?appId=61e5835e5fbce3003a653ee6&number=424&type=linked&view=changes)

* in the kaleidos project there is one instance where we use splattributes so this needs checking as it might break (@brenner-company maybe you have a better view on that?).

In general the inlining of separate svg's has a disadvantage over the use method. If you have a table where each row includes an icon you have to repeat the svg code multiple times. On long tables or multiple repetitions this will increase the file size quite a lot. Is inlining the svg symbolset not an option with svg-jar?

elpoelma commented 1 year ago

Moving the icon classes to the span did not help. Removing the span alltogether does fix the issue.

elpoelma commented 1 year ago

I have also thought about the following approach, in which we use the utility functions of the ember-svg-jar package.

function getInlineAsset(assetId) { let result = null; try { result = require(ember-svg-jar/inlined/${assetId}).default; } catch (err) { // skip } try { result = importSync(../inlined/${assetId}).default; } catch (err) { // skip } return result; }

export default class AuButton extends Component { get size() { if (this.args.size == 'large') return 'au-c-icon--large'; else return ''; }

get alignment() { if (this.args.alignment == 'left') return 'au-c-icon--left'; if (this.args.alignment == 'right') return 'au-c-icon--right'; else return ''; }

get asset(){ return getInlineAsset(this.args.icon); }

get viewBox(){ return this.asset.attrs.viewBox; }

get height(){ return this.asset.attrs.height; }

get width(){ return this.asset.attrs.width; }

get content(){ return htmlSafe(this.asset.content); } }



The advantage of this approach is that we can keep using the splattributes (so the API isn't breaking). 
In this situation we can also modify it so the user can easily choose between the symbolset or inline approach.
Windvis commented 1 year ago

I have also thought about the following approach, in which we use the utility functions of the ember-svg-jar package.

I don't think we can consider those utils public API, so I wouldn't do this. Maybe we could try to find out why they use a helper instead of a component? Having a component would make things a lot easier for us here.

It is not possible to pass splattributes to the svg-jar helper provided by ember-svg-jar. This PR introduces an additional span element which accepts the splattributes around the svg-jar helper. However, when searching for the usage of the AuIcon component in the projects using ember-appuniversum no usage of these splattributes on au-icon is found.

I think we use it in some places in Appuniversum to pass some data-test attributes, but we could refactor those. Kaleidos does also seem to use it in 1 place so not sure if we can actually drop this.

elpoelma commented 1 year ago

For now the project which requires inline svgs (https://github.com/lblod/frontend-embeddable-notule-editor) will override the au-icon component using the svg-jar package.

brenner-company commented 1 year ago
  • in the kaleidos project there is one instance where we use splattributes so this needs checking as it might break (@brenner-company maybe you have a better view on that?).

@Dietr with @elpoelma proposition at https://github.com/appuniversum/ember-appuniversum/pull/424#issuecomment-1715589311, do I still need to check the possible consequences of the changes within Kaleidos? I don't think so, but I wanted to check to be sure.

Dietr commented 1 year ago

@brenner-company No, not needed for now.

Windvis commented 5 months ago

Closing since we went for an alternative solution in #483 and #481