-
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(icon): add control for icon title - FRONT-4399 #3414
Conversation
@@ -97,8 +98,16 @@ | |||
{%- if icon.extra_classes is defined and icon.extra_classes is not empty -%} | |||
{% set _icon_extra_classes = _icon_extra_classes ~ ' ' ~ icon.extra_classes %} | |||
{%- endif -%} | |||
{% set _icon_extra_accessibility = {} %} |
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.
mmhh..isn't it better if we do this in the icon template? I mean that the user would normally be able to set this title passing it as part of the extra_accessibility, we do what is in this pr i guess in order to "simplify" the data structure and let the user pass a "title" prop in the main icon object, instead. But then we have to do this for all the components including the icon, basically, why this could be an option available at the icon template level, so that we could check for this "simplified" title and use it in case it is present while still relying normally on the extra_accessibility object for that.
{% set _css_class = 'ecl-icon' %} | ||
{% set _extra_attributes = '' %} | ||
|
||
{# Internal logic - Process properties #} | ||
|
||
{% if _icon.title is not empty or _extra_accessibility.title is not empty %} |
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.
Since we have already "merged" the _icon.title in the _extra_accessibility here we could simply check for the latter
name: 'icon title', | ||
type: 'string', | ||
description: 'Textual information for the icon, mostly for screen readers', | ||
if: { arg: 'external', truthy: false }, |
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..we just investigated the possibility of defining multiple conditions and unfortunately that is not currently possible. With this syntax basically only the last condition is considered, so to achieve this we should kind of duplicate the control splitting the conditions. But it seems unneeded here, we are not able to build the perfect story, but the issues we have are minor, when an icon is selected and then the external control is activated we do have unneeded controls for the icon (transform, but probably also position), when the external is set to true and no icon was previously chosen then no additional control (so even the icon title) will be visible.
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.
The hide_label param is missing an if for the icon_name not to be 'none'.
title
has been added for this. It was already possible before, but required to change several parameters (previous way is still working).