Restore tooltips on si-navbar-vertical and si-content-action-bar#2034
Restore tooltips on si-navbar-vertical and si-content-action-bar#2034fabrice-aeschbacher wants to merge 1 commit into
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a tooltip property to menu and navbar models and updates the content action bar and vertical navbar components to display these tooltips. Documentation was also added to outline best practices for tooltip accessibility. Feedback focuses on improving documentation phrasing and optimizing the siTooltip implementation in the content action bar by using the [isDisabled] input to prevent redundant ARIA attributes and tooltip instances.
| [attr.data-id]="primaryAction.id" | ||
| [attr.aria-label]="primaryAction.label | translate" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : ''" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" |
There was a problem hiding this comment.
According to the general rules, the siTooltip directive should be disabled using the [isDisabled] input instead of setting the content to an empty string. This prevents the creation of unnecessary tooltip instances and ARIA attributes.
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" | |
| [siTooltip]="(primaryAction.iconOnly ? primaryAction.label : (primaryAction.tooltip ?? '')) | translate" | |
| [isDisabled]="!primaryAction.iconOnly && !primaryAction.tooltip" |
References
- To disable the siTooltip directive, use the [isDisabled] input. Avoid setting the content to an empty string, as this still creates a tooltip instance with ARIA attributes.
| [attr.data-id]="primaryAction.id" | ||
| [attr.aria-label]="primaryAction.label | translate" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : ''" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" |
There was a problem hiding this comment.
According to the general rules, the siTooltip directive should be disabled using the [isDisabled] input instead of setting the content to an empty string.
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" | |
| [siTooltip]="(primaryAction.iconOnly ? primaryAction.label : (primaryAction.tooltip ?? '')) | translate" | |
| [isDisabled]="!primaryAction.iconOnly && !primaryAction.tooltip" |
References
- To disable the siTooltip directive, use the [isDisabled] input. Avoid setting the content to an empty string, as this still creates a tooltip instance with ARIA attributes.
| [attr.data-id]="primaryAction.id" | ||
| [attr.aria-label]="primaryAction.label | translate" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : ''" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" |
There was a problem hiding this comment.
According to the general rules, the siTooltip directive should be disabled using the [isDisabled] input instead of setting the content to an empty string.
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" | |
| [siTooltip]="(primaryAction.iconOnly ? primaryAction.label : (primaryAction.tooltip ?? '')) | translate" | |
| [isDisabled]="!primaryAction.iconOnly && !primaryAction.tooltip" |
References
- To disable the siTooltip directive, use the [isDisabled] input. Avoid setting the content to an empty string, as this still creates a tooltip instance with ARIA attributes.
| [attr.data-id]="primaryAction.id" | ||
| [attr.aria-label]="primaryAction.label | translate" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : ''" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" |
There was a problem hiding this comment.
According to the general rules, the siTooltip directive should be disabled using the [isDisabled] input instead of setting the content to an empty string.
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" | |
| [siTooltip]="(primaryAction.iconOnly ? primaryAction.label : (primaryAction.tooltip ?? '')) | translate" | |
| [isDisabled]="!primaryAction.iconOnly && !primaryAction.tooltip" |
References
- To disable the siTooltip directive, use the [isDisabled] input. Avoid setting the content to an empty string, as this still creates a tooltip instance with ARIA attributes.
| [attr.data-id]="primaryAction.id" | ||
| [attr.aria-label]="primaryAction.label | translate" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : ''" | ||
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" |
There was a problem hiding this comment.
According to the general rules, the siTooltip directive should be disabled using the [isDisabled] input instead of setting the content to an empty string.
| [siTooltip]="primaryAction.iconOnly ? (primaryAction.label | translate) : (primaryAction.tooltip ?? '' | translate)" | |
| [siTooltip]="(primaryAction.iconOnly ? primaryAction.label : (primaryAction.tooltip ?? '')) | translate" | |
| [isDisabled]="!primaryAction.iconOnly && !primaryAction.tooltip" |
References
- To disable the siTooltip directive, use the [isDisabled] input. Avoid setting the content to an empty string, as this still creates a tooltip instance with ARIA attributes.
fabrice-aeschbacher
force-pushed
the
feat/menuitem-tooltip
branch
from
f68fc40 to
253b2d7
Compare
|
@panch1739 Can you have a look here, if we want to have a tooltip in the components I can support Fabrice here |
spliffone
added
the
core-team-discussion
kfenner
removed
the
core-team-discussion
fabrice-aeschbacher
force-pushed
the
feat/menuitem-tooltip
branch
from
253b2d7 to
d884f00
Compare
MenuItem from @simpl/element-ng/menu has been deprecated in v47.0.0, but the new implementation removed the tooltips. This patch restores the feature, while fully respecting the new implementation in case no optional tooltip is given. Closes siemens#1259 Signed-off-by: Fabrice Aeschbacher <fabrice.aeschbacher@siemens.com> Reviewed-by: Gwendoline Goetz <gwendoline.goetz@siemens.com>
fabrice-aeschbacher
force-pushed
the
feat/menuitem-tooltip
branch
from
d884f00 to
7aea90d
Compare
5 participants
MenuItem from @simpl/element-ng/menu has been deprecated in v47.0.0, but the new implementation removed the tooltips. This patch restores the feature, while fully respecting the new implementation in case no optional tooltip is given.