-
Notifications
You must be signed in to change notification settings - Fork 24
Swapping icons to Font Awesome, improves icon accessibility #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crispy
I like how much file clutter this PR reduces and there are some weird ways we've done things that have been refactored out by the changes in this PR.
Left one clarifying comment
@@ -13,7 +13,7 @@ function Footer(){ | |||
</div> | |||
<div className="footer-item"> | |||
<h3>Reach us at</h3> | |||
<a href="mailto: [email protected]" className="email"><span>[email protected]</span></a> | |||
<a href="mailto: [email protected]" className="email"><span className="footer-text">[email protected]</span></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what the forcing function is for this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great q! It's because there's a very generic span
selector in this line of this file that was being applied to the buttons inadvertently, so I specified it. There's no impact on the final render for the non-icons.
Thanks for the reviews y'all ❤️ |
This PR does a couple of things: * update `react-scripts` (the package for create-react-app) to `v4.0.3` - this is a major breaking change, but also helps with #130 * update `eslint` to v7+, to resolve newly-created peer-dependency mismatches * removes some unused CSS I forgot to remove from #148 * changes the about diamond to a `src/assets`-imported image rather than using a `background-image` hack (the nice externality is that it's also more accessible)! Closes #136.
Concisely, this PR does a couple of things:
aria-label
tags to make icon links accessiblepublic/
folderThis PR is related to three issues:
public/
folder)The components/pages we hit are:
<SocialMedia/>
CTA, which is on every page - needed to also adjust some in specific CSS<SocialMedia/>
CTAs on the home page and the about pageAnd some other relevant notes:
1.02
), I change the color of the link. I feel that this is easier to see (and thus more accessible), plus causes less layout jank.Let me know what y'all think 😊