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

USWDS-Site: Fix token pages from audit (Part 2) #2377

Merged
merged 26 commits into from
May 15, 2024
Merged

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Nov 27, 2023

Summary

Updated component pages according to the findings reported in the code/developer content audit (Google Docs 🔒)

Note

To keep PR reviews manageable, this PR updates only the theme, state, and system color tokens pages. All other token page updates will be addressed in other PRs.

Important

We should confirm the changelog dates before merging.

Related issue

Related to #2042

Preview link

Preview link:

Solution

Update code according to the findings in code/developer content audit (Google Docs 🔒).

Please note that all items highlighted yellow in the spreadsheet will be investigated/discussed in the last phase of the component page fixes.

Testing and review

  • Confirm that all updates are accurate and make sense
  • Confirm that all meaningful updates have a related changelog
    • "Meaningful" updates here mostly applied to adding or removing documentation about variants, dependencies, etc. Copy tweaks to add clarity were not considered meaningful, but a case could be made for them to be included.
  • Confirm grammar and spelling for all updates

@amyleadem amyleadem linked an issue Nov 27, 2023 that may be closed by this pull request
@amyleadem amyleadem marked this pull request as ready for review November 27, 2023 23:50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

These entries could be condensed into one if that's what we prefer.

Comment on lines -170 to -173
- token: emergency
default: "red-warm-60v"
- token: emergency-dark
default: "red-warm-80"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

Removed these from theme colors because they are already listed in state colors

- Tweak content in customize-color-token.html
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Changes are logical improve documentation. All green changes in the doc are present and match what is found in USWDS.

I left a comment about the color mixin palettes for further investigation but changes lgtm!

Testing checklist

  • Theme colors page
    • Update _uswds_theme_color.scss reference to settings config
    • secondary-lighter value should be red-cool-10
    • Remove emergency tokens from the colors page
    • I don’t see $theme-border-color in USWDS
  • State colors page
    • Getting error warning-dark is not a valid USWDS token
    • Update _uswds_theme_color.scss reference to settings config
  • System colors page
    • Not showing -90v items on site
    • -red-50v Not working with color utilities. System color palette not being included in the utility map.
  • Changelogs look good

@@ -294,7 +280,7 @@ Your context and coding style determine how you access USWDS color tokens in cod
</td>
<td data-title="Example">
<span>
.bg-<code>red-50v</code>
.bg-<code>gray-30</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Color utility palettes

It looks like the color and background utility color palettes aren't importing the system colors to use in color- or bg- mixins.

From what I can tell it's only using $palette-color-default

  "palette-color-default":
    map-collect(
      $tokens-color-basic,
      $tokens-color-grayscale,
      $tokens-color-theme,
      $tokens-color-state
    ),

But the background color utilty does reference red-50v as an example value.

https://github.com/uswds/uswds/blob/3175c0258a63e6e464ea8284bb90f452f46463a1/packages/uswds-utilities/src/styles/rules/background-color.scss#L10-L14

Should we create an issue to investigate this further and see if this change was intentional?

@amyleadem amyleadem self-assigned this Dec 14, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

The system does not make the listed utility classes for red-50v. I have replaced these examples with gray-30, which is universally applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot of table

image

Comment on lines -261 to -279
$theme-color-warning: <code>'red-50v'</code>
</span>
</td>
</tr>
<tr>
<th scope="row" data-title="Context">
<span class="font-lang-3">
<span>mixin</span><br/>
<span class="text-normal">text-decoration-color</span>
</span>
</th>
<td data-title="Description">
<span>
@include u-underline(<a href="{{ site.baseurl }}/design-tokens/color/state-tokens/" class="token">color</a>)
</span>
</td>
<td data-title="Example">
<span>
@include u-underline(<code>'red-50v'</code>)<br/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

Removed this row because it was a duplicate of a previous row

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot of table

image

@mejiaj

This comment was marked as resolved.

- This should resolve snyk error
@amyleadem
Copy link
Contributor Author

@mejiaj I resolved the snyk errors 👍. All I see is our standard pa11y crashes atm.

@amyleadem amyleadem added this to the 2024.04 April milestone Apr 3, 2024
type: token
changelogURL:
items:
- date: 2024-04-02
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Important

Need to update the changelog dates before merge

@brunerae brunerae modified the milestones: 2024.04 April, 2024.05 May May 13, 2024
Copy link
Member

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Thank you!

@thisisdano
Copy link
Member

Merging over circle crashes

@thisisdano thisisdano merged commit f4162ca into main May 15, 2024
9 of 11 checks passed
@thisisdano thisisdano deleted the al-tokens-audit-2 branch May 15, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

USWDS-Site - Code audit: Fix inaccuracies in tokens pages
5 participants