feat(link): warn if link opens in new tab but no tooltip or label for icon is provided #710
Conversation
There was a problem hiding this comment.
suggestion:
Line 234: could you add a tooltipContent prop to silence the console warn
There was a problem hiding this comment.
suggestion:
could you add a test to check that it outputs the warning
There was a problem hiding this comment.
issue: Overflow is broken
| pr | master |
|---|---|
 |
 |
| </div> | ||
| </a> | ||
| ); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
thanks for simplifying it
| @@ -37,12 +37,12 @@ const Link = forwardRef((props: Props, providedRef: RefObject<HTMLAnchorElement> | |||
|
|
|||
There was a problem hiding this comment.
we should add a warn here:
if(isExternalLink && !hasExternalLinkIcon) {
console.warn(
'MRV2: The link opens a new tab and it has no external link icon indicating this. Please pass hasExternalLinkIcon: true to the icon'
}
There was a problem hiding this comment.
if it's external link and hasExternalLinkIcon is undefined, it'll automatically render the icon
whether the icon is rendered is not solely based on the hasExternalLinkIcon prop
| isShowIcon = true; | ||
| } else if (hasExternalLinkIcon) { | ||
| isShowIcon = true; | ||
| if (isShowIcon && !tooltipContent && !externalLinkIconProps?.['aria-label']) { |
There was a problem hiding this comment.
we are allowing the developer to have an icon that is not labelled visibly, only with aria-label. So that violates one of the icon rules.
Maybe it could be:
const isShowExternalIcon = hasExternalLinkIcon || (hasExternalLinkIcon === undefined && isExternalLink);
if (isShowIcon && !tooltipContent) {
console.warn(
'MRV2: This link is external link, so an indicator icon will be shown next to it. All meaningful icons need a tooltip, so please pass tooltipContent="Opens a new tab" to the link'
);
}
| {!tooltipContent && <a {...commonProps}>{props.children}</a>} | ||
| </> | ||
| {tooltipContent ? ( | ||
| <Tooltip type={tooltipType} placement="bottom" triggerComponent={content}> |
There was a problem hiding this comment.
We should default our tooltipType to "description", so that the SR reads '{content of the link}, Opens a new tab, Link'. Maybe we should not even allow the user to pass a tootlipType so that this is the only allowed behaviour
IMPORTANT
Currently, this project is closed to any external contributions. Any pull request made against this project from external sources will likely be closed. If you would like to make changes to this project, please fork this project.
Guide
This "Help" section can be deleted before submitting this pull request.
Update the name of this pull request to reflect the following shape:
Provide a general summary of the scope of the changes in this pull request.
Description
see title
Links
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-582981