Skip to content
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

Footer update #96

Merged
merged 3 commits into from
Mar 15, 2023
Merged

Footer update #96

merged 3 commits into from
Mar 15, 2023

Conversation

nmakuch
Copy link
Contributor

@nmakuch nmakuch commented Mar 13, 2023

This PR consists of the following work:

Updated some footer tokens for this PR

Copy link
Collaborator

@ethanWallace ethanWallace left a comment

Choose a reason for hiding this comment

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

Just a few questions for now

@@ -122,7 +122,7 @@
},
"hover": {
"text": {
"value": "{link.hover.value}",
"value": "{color.blue.900.value}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you switched from the global link-hover token? They both seem to use blue.900

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops, looks like I should have left it as link.hover

@@ -169,7 +169,7 @@
}
},
"text": {
"value": "{link.default.value}",
"value": "{color.blue.800.value}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we switched from the link.default token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one, however, needs to change to 800 instead of 700. with the gray bg it doesn't meet AAA contrast standards and bumping it up to 800 fixes the contrast ratio.

Should I make a new token for this one or is it fine as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine as is

@nmakuch nmakuch requested a review from ethanWallace March 14, 2023 21:56
Copy link
Collaborator

@ethanWallace ethanWallace left a comment

Choose a reason for hiding this comment

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

LGTM

@nmakuch nmakuch merged commit 3f080fc into main Mar 15, 2023
@nmakuch nmakuch deleted the footer-update branch March 15, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants