-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(news-ticker): Icon or image for each item, iteration on styles for EC and EU - FRONT-4463 #3447
Conversation
…ntermediate screens - FRONT-4463
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{% include '@ecl/icon/icon.html.twig' with { | ||
icon: _item.icon|merge({ | ||
path: _icon_path, | ||
name: _item.icon.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we default the icon name to 'information' (at least until next major version)?
That way there won't be breaking changes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh..i'd say no, because we would alter the behavior of the component, currently you can set or not an icon for each item, if we default it to "something" it would always be there instead, and it would be totally arbitrary to show the info icon at this point. It would not work as backward compatibility, for that to happen we should check if in any of the item there is an icon and in case there is none we could "assume" that it's "old data" and apply this fallback but it seems an overhead for the template to do so.
@@ -27,20 +28,25 @@ $news-ticker: null !default; | |||
.ecl-news-ticker__icon { | |||
fill: var(--c-p); | |||
flex-shrink: 0; | |||
margin-inline-start: map.get($news-ticker, 'padding-horizontal'); | |||
height: 1.5rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use the sass maps for the icon size
link: exampleLink, | ||
icon: { | ||
name: 'euro', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should have different icons on some slides, to showcase this possibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done it, although for some reasons i was explicitely requested to use the euro icon..
…component-library into FRONT-4463-News-ticker
No description provided.