feat(icon): Webtools icons - FRONT-4688 #3736
Conversation
emeryro
changed the title
…uropa-component-library into FRONT-4688-poc-icons
There was a problem hiding this comment.
I think we don't need to expose a parameter in each component using icons, it's a lot of code and logic to handle something that should belong to the icon, not to the component implementing it. Also, if the intention is to always use the other icons, then it would be just a matter of toggling the value of the parameter in the icon template and all these additional parameters would become obsolete and useless.
| @@ -15,6 +15,7 @@ npm install --save @ecl/twig-component-accordion | |||
| - "label" (string) (default: '') | |||
| - "content" (string) (default: '') | |||
| - **"icon"** (array) OR (object) (default: []) Two icons in an array that will be toggled | |||
| - **"icon_wt_markup"** (boolean) (default: false): should the icon use the Webtools markup? | |||
There was a problem hiding this comment.
it's a boolean but it mentions markup in its name, feels confusing, anyway do we need it at all? I understand that in certain components we might be defining the icons ourselves, and there we need somehow to add a param to handle this, but here and in many other components we could simply use the param as part of the icon object passed. Also, it might not really be a real use case, but if i want to use one icon only from webtools in components that have multiple it would not be possible this way.
| @@ -65,7 +72,7 @@ | |||
| {% endif %} | |||
|
|
|||
| {% for property in ['size', 'transform', 'color'] %} | |||
| {% if _icon[property] is defined and _icon[property] is not empty %} | |||
| {% if _icon[property] is defined and _icon[property] is not empty and _icon[property] != 'none' %} | |||
There was a problem hiding this comment.
is this additional check needed because of the storybook demo..?
| {% set _wt_classes = '' %} | ||
| {% set _full_name = '' %} | ||
| {% if _icon.category is not empty %} | ||
| {% set _full_name = _full_name ~ '-' ~ _icon.category %} |
There was a problem hiding this comment.
this is weird, _full_name is an empty string, guess that it can simply get _icon_category as its value when its present.
| @@ -19,25 +19,36 @@ const getArgs = (data) => ({ | |||
| color: 'default', | |||
| transform: 'none', | |||
| title: '', | |||
| description: '', | |||
There was a problem hiding this comment.
not sure about the whole plan regarding this, but shouldn't we expose a control to demo this?
| @@ -77,10 +79,11 @@ | |||
| {% set _link = _link|merge(link) %} | |||
| {% endif %} | |||
|
|
|||
| {% if _link.external and _link.icon_path is not empty %} | |||
| {% if _link.external and (_link.icon_path is not empty or _link.icon_wt_markup) %} | |||
There was a problem hiding this comment.
this is one of those cases where i don't understand why adding the parameter at the component level, we merge the data provided by the user, why do we need to add this complexity and additional processing with also the consequence of forcing all the icons to follow the same approach?
|
I'll check the different points, but to address your main concern: |
Add possibility to use Webtools icons instead of the ones provided directly by ECL
Main ideas:
wt_markup. Setting it to true would render the webtools markup, later replaced by the icon via Webtools javascripticon_wt_markup. This had to be done at component level, as some icon are handled directly here, and do not expose the full icon structure