Skip to content

docs: underline URLs, change contrast in syntax highlighting #8225

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

Merged
merged 13 commits into from
Jan 25, 2024

Conversation

lucas-amberg
Copy link
Contributor

@lucas-amberg lucas-amberg commented Jan 8, 2024

PR Checklist

Overview

For URL Underlines: Updated CSS to add underlines to URLs and added another rule to remove underlines from the table of contents, next page buttons, edit page button, and menu sidebars along with the social media links in the footer (as they can be distinguished with the external link icon).

For Syntax Highlighting: Checked axe Devtools on multiple pages of documentation to find contrast issues in syntax highlighting and adjusted CSS to improve contrast ratio to be >4.5:1 as specified.

Include next page in comment as I forgot to include it beforehand
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @lucas-amberg!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Jan 8, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit a26d262
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65ac18ae03caba000885d4c3
😎 Deploy Preview https://deploy-preview-8225--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 91 (🔴 down 6 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@JoshuaKGoldberg
Copy link
Member

Btw @lucas-amberg is there a reason you closed #8217 and sent this new one?

In case this is useful info: you don't have to delete a PR then re-make it every time you have a new commit 🙂. You can push commits to the same branch.

I'd also recommend sending from a branch other than main. https://stackoverflow.com/questions/60631439/why-is-it-bad-practice-to-submit-pull-request-from-master-branch (replace "master" with "main")

@lucas-amberg
Copy link
Contributor Author

The reason I closed it was just so I could restart with a clean fork, I'm sorry for the confusion.

Also thank you for the recommendation, I'll make sure to do that in the future. I appreciate the help as I'm really new to this stuff 😅.

@JoshuaKGoldberg
Copy link
Member

Ooh! Welcome to Git / GitHub / open-source / whatever-combination-of-those-is-new-for-you! 😄🙌

You're actually doing quite well with adhering to standards (the PR is titled well, addresses an accepted+open issue, etc.). This is great!

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Progress! 🚀

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 8, 2024
@lucas-amberg
Copy link
Contributor Author

This route would've been a lot easier

@lucas-amberg lucas-amberg changed the title docs: underline URLs docs: underline URLs, change contrast in syntax highlighting Jan 8, 2024
Copy link
Contributor

@StyleShit StyleShit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to the open-source world!

Looks good to me :)

Comment on lines +9 to +18
--token-color-symbol: #277c7b;
--token-color-number: #098658;
--token-color-keyword: #00f;
--token-color-function: #569cd6;
--token-color-function: #2b74b1;
--token-color-function-variable: #000;
--token-color-important: #e90;
--token-color-class-name: #2b91af;
--token-color-class-name: #237690;
--token-color-selector: #800000;
--token-color-regexp: #800000;
--token-color-property: #d00;
--token-color-property: #c70000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Curious]

How did you choose those specific colors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran axe devtools to see which colors on each page were below the 4.5:1 threshold and then I used this website to check the contrast ratio and modify the current foreground color until it was over that amount. Then I re-ran the devtools to verify the changes were adequate 🙂.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool! Thanks for working on this! 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[tip] Try to avoid fixing several unrelated issues in one PR, even if they are small. This is generally a good practice + it helps maintainers better focus on reviewing fixes for a particular issue.

If two issues are about the same bug/feature, then they should be merged into one issue. Thus, one issue = one PR

@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 22, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating with us on this!

Joe Pesci dressed as a cop in Home Alone 2 saying 'Excellent. Excellent.'

@JoshuaKGoldberg JoshuaKGoldberg merged commit 24f9d4c into typescript-eslint:main Jan 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants