[Issue #440] add github CTA to Hero #441
Conversation
frontend/src/components/Hero.tsx
Outdated
| href={ExternalRoutes.GITHUB_REPO} | ||
| target="_blank" | ||
| > | ||
| <Icon.Github className="usa-icon usa-icon--size-3 margin-right-1 text-middle tablet:margin-top-neg-2px" /> |
There was a problem hiding this comment.
Using the Truss Icon component gives this error:
"<svg> elements with an img role must have an alternative text (svg-img-alt)"
…which was reported in the Truss repo, which indicates it's working as-intended.
Not sure how to resolve this.
There was a problem hiding this comment.
@SammySteiner @daphnegold … halp pleeeze
There was a problem hiding this comment.
Oh, you just need to add aria-label. I'm pushing a fix upstream now.
| @@ -8,14 +11,22 @@ const Hero = () => { | |||
|
|
|||
| return ( | |||
| <div data-testid="hero" className="usa-dark-background bg-primary"> | |||
| <GridContainer className="padding-y-1 tablet:padding-y-3 tablet-lg:padding-y-10 desktop-lg:padding-y-15"> | |||
| <GridContainer className="padding-y-1 tablet:padding-y-3 tablet-lg:padding-y-10 desktop-lg:padding-y-15 position-relative"> | |||
There was a problem hiding this comment.
Added positioning so that the child link can be positioned relative to this element.
| $position-settings: ( | ||
| output: true, | ||
| responsive: true, | ||
| active: false, | ||
| focus: false, | ||
| hover: false, | ||
| visited: false | ||
| ), |
There was a problem hiding this comment.
This is required to add positioning only on larger screens. I'm only changing the responsive values, but it only seems to work if you include all position settings. I think this happened before with breakpoints. I'm confused as to when you can override an individual setting and when you have to include all. Maybe we can get clarity on this?
There was a problem hiding this comment.
I'm not sure either, that's so interesting. I thought you'd be able to do that.
| <h1 className="tablet:font-sans-2xl desktop-lg:font-sans-3xl text-ls-neg-2"> | ||
| <span>{t("title")}</span> | ||
| </h1> | ||
| <p className="usa-intro font-sans-md tablet:font-sans-lg desktop-lg:font-sans-xl"> | ||
| <span className="text-yellow text-bold">{t("beta")}</span> | ||
| {t("content")} | ||
| </p> | ||
| <Link | ||
| className="usa-button usa-button--outline usa-button--inverse font-sans-2xs tablet:font-sans-md margin-bottom-3 desktop:position-absolute top-3 right-0 desktop:margin-y-3 desktop:margin-x-4" |
There was a problem hiding this comment.
A lot of classes here, but it's basically a small link on small screens, then on larger screens it's a bit bigger and positioned at the top-right of the Hero.
| <h1 className="tablet:font-sans-2xl desktop-lg:font-sans-3xl text-ls-neg-2"> | ||
| <span>{t("title")}</span> | ||
| </h1> | ||
| <p className="usa-intro font-sans-md tablet:font-sans-lg desktop-lg:font-sans-xl"> | ||
| <span className="text-yellow text-bold">{t("beta")}</span> | ||
| {t("content")} | ||
| </p> | ||
| <Link |
There was a problem hiding this comment.
Not really a particular reason to use Link here since it's navigating off page, but doesn't matter, I think if we use a or Link tags. Not a change request, just pondering out loud. :D
| $position-settings: ( | ||
| output: true, | ||
| responsive: true, | ||
| active: false, | ||
| focus: false, | ||
| hover: false, | ||
| visited: false | ||
| ), |
There was a problem hiding this comment.
I'm not sure either, that's so interesting. I thought you'd be able to do that.
Summary
Fixes #440
Time to review: 1 mins
Changes proposed
Context for reviewers
per Lucas's request to make this call-to-action more prominent
Additional information