Use Link component instead of anchor tag in Code Docs #45010
Conversation
There was a problem hiding this comment.
re: the styling difference -- the color change is definitely intentional, and i think the weight change you made here is appropriate (i.e., the programming expression names used to be a higher weight and are just "regular" now). there doesn't seem to be a good design reason to have all the expression names be medium/bold
| @@ -12,7 +13,9 @@ export default function CodeDocLink({programmingExpression}) { | |||
| ); | |||
| } else { | |||
| return ( | |||
| <a href={programmingExpression.link}>{programmingExpression.name}</a> | |||
| <Link href={programmingExpression.link} weight="regular"> | |||
There was a problem hiding this comment.
"regular" is the default for the weight prop, so you can remove it unless you want to make sure the weight is never affected
| @@ -1,6 +1,7 @@ | |||
| import PropTypes from 'prop-types'; | |||
| import React from 'react'; | |||
| import EmbeddedBlock from '@cdo/apps/templates/codeDocs/EmbeddedBlock'; | |||
| import {Link} from '@dsco_/link'; | |||
There was a problem hiding this comment.
this is more a question for feedback than anything, but is there a reason you used Link instead of TextLink in both places? in this case, there's no difference since you aren't using an icon with your link, but i'm interested if there is (or will be) a usage preference. the difference would basically be:
// what you have here
<Link href={programmingExpression.link} weight="regular">
{programmingExpression.name}
</Link>
// alternative with TextLink
<TextLink
href={programmingExpression.link}
text={programmingExpression.name}
weight="regular"
/>There was a problem hiding this comment.
ahhh I didn't realize the difference and got a bit distracted by the icon functionality. I think TextLink is better so I'll use that
There was a problem hiding this comment.
This also helped clarify that I can use Link for my non-text link. So I updated this PR to do that too. Not really much gained afaict but I'm already here
| @@ -23,13 +24,13 @@ export default function EmbeddedBlock({blockName, link}) { | |||
|
|
|||
| return ( | |||
| <div> | |||
| <a href={link}> | |||
| <Link href={link}> | |||
There was a problem hiding this comment.
i'm going to try to sneak in some small accessibility wins with this 😄 right now, screenreaders will read this element as "[not] visited, link, [whatever-href-is]", but if you add an "aria-label" to the child (since the child isn't a text node), the screenreader output will be more digestible: "[not] visited, link, [aria-label]"
an example is probably easier:
// screenreader: "visited, link, Go to Code Studio"
// anything that can resolve to a text node doesn't need an aria-label
<Link href="https://studio.code.org">Go to Code Studio</Link>
// screenreader: "visited, link, https://studio.code.org"
// what we have here
<Link href="https://studio.code.org">
<div />
</Link>
// screenreader: "visited, link, Go to Code Studio"
<Link href="https://studio.code.org">
<div aria-label="Go to Code Studio" />
</Link>in this case, maybe the label should be blockName?
we may want to enforce this in <Link> at some point, but it's kind of a gray area because aria-label isn't always required.
There was a problem hiding this comment.
Yes good thought! thanks!
There was a problem hiding this comment.
I went a bit farther and added a prop to pass in the name of the expression, which is a bit more human readable
Switching to using the
Linkcomponent from dsco instead of anchor tags in code docs.This did cause a bit of a style change, but I assume that's intended.
Before:
After:
Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: