refactor: only show external icon for link + button when external prop is used#689
refactor: only show external icon for link + button when external prop is used#689melaniebmn wants to merge 3 commits into
Conversation
| }); | ||
| expect(root).toEqualHtml(` | ||
| <gcds-link href="https://google.com" target="_blank" lang="fr"> | ||
| <gcds-link href="https://google.com" external lang="fr"> |
There was a problem hiding this comment.
We already have it added to the index.html below, but can we also add it to the test? Let's add one test for:
// This link opens in a new tab, but is not external.
<gcds-link target="_blank"> | /** | ||
| * Whether the link is external or not | ||
| */ | ||
| @Prop() external?: boolean = false; |
There was a problem hiding this comment.
Because this is a prop change, I think it would be a good idea to be specific here since it only applies if the button is type="link"
| /** | |
| * Whether the link is external or not | |
| */ | |
| @Prop() external?: boolean = false; | |
| /** | |
| * Set to true if the button is meant to link to an external page. Requires property `type` to be set to `"link"` for this to work. | |
| */ | |
| @Prop() externalLink?: boolean = false; |
| }); | ||
| expect(root).toEqualHtml(` | ||
| <gcds-button type="link" button-style="text-only" href="https://google.com" target="_blank"> | ||
| <gcds-button type="link" href="https://google.com" external> |
There was a problem hiding this comment.
Can we also add a test for "target=_blank", when we are not displaying the external icon? For internal pages that we want to open on another page? Is that possible? I'm thinking it's the same behaviour for the link.
daine
left a comment
daine
left a comment
There was a problem hiding this comment.
Just one last item that we discussed during sync earlier
| * Set to true if the button links to an external page. | ||
| * Requires the "type" to be set to "link" to work. | ||
| */ | ||
| @Prop() external?: boolean = false; |
There was a problem hiding this comment.
Is it possible to separate the new prop from this PR so we don't introduce a change to this component for now? We can add a prop in a separate PR and go through a component update process.
3 participants
Summary | Résumé
While working on the new page templates for our docs site, I saw an opportunity to improve the behavior of the link and button components, specifically regarding the display of the external icon. Previously, the external icon was shown whenever the
target="_blank"attribute was used, but it should only appear for links pointing to external, non-canada.ca pages. I updated the code to display the external icon only when theexternalprop is present on either component. This change now allows users to settarget="_blank"on internal links without triggering the external icon, ensuring more accurate and consistent behavior.