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

Convert Less to Sass #8514

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Convert Less to Sass #8514

merged 7 commits into from
Aug 22, 2024

Conversation

anselmbradford
Copy link
Member

@anselmbradford anselmbradford commented Jul 16, 2024

Changes

How to test this PR

  1. yarn install && yarn build and site should be unchanged.

@anselmbradford anselmbradford force-pushed the ans_sass branch 4 times, most recently from b6adba5 to 445e41c Compare July 25, 2024 12:59
@anselmbradford anselmbradford force-pushed the ans_sass branch 4 times, most recently from 8f3c727 to aacae5e Compare August 1, 2024 14:39
@anselmbradford anselmbradford marked this pull request as ready for review August 1, 2024 14:44
@anselmbradford anselmbradford requested a review from wpears August 1, 2024 14:44
@anselmbradford anselmbradford force-pushed the ans_sass branch 4 times, most recently from 8b0a901 to 0f63b25 Compare August 2, 2024 16:36
Comment on lines 131 to 135
$tint: #{$value}-bg;
$light: #{$value}-lt;
$med: #{$value}-md;
Copy link
Member

Choose a reason for hiding this comment

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

Needs to read from colors map:

Suggested change
$tint: #{$value}-bg;
$light: #{$value}-lt;
$med: #{$value}-md;
$tint: map.get($colors-map, #{$value}-bg);
$light: map.get($colors-map, #{$value}-lt);
$med: map.get($colors-map, #{$value}-md);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

Nice job! Re: our chat about system-ui, I've noticed that some of our apps (TCCP, find a housing counselor) are prioritizing system-ui over Avenir Next, causing the font to be slightly different. Not sure why but something to be aware of.

image

@wpears
Copy link
Member

wpears commented Aug 13, 2024

I've noticed that some of our apps (TCCP, find a housing counselor) are prioritizing system-ui over Avenir Next

This seems like it's because Avenir is only defined in cfpb-typography, vs. system-ui being defined in cfpb-core and both of those apps import core but not typography.

I imagine that sass is choking on it in a way that a less reference import isn't.

Perhaps @use is the solution here?

@anselmbradford
Copy link
Member Author

Updated to v3 DS. Try it now. Thanks!

@anselmbradford
Copy link
Member Author

anselmbradford commented Aug 16, 2024

Erp I'll need to fix the jest tests so they don't try to scoop up the scss files, but the site can still be checked locally.

EDIT: fixed it with moduleNameMapper config.

Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

Tests and looks great!

@anselmbradford anselmbradford added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 6dde6fb Aug 22, 2024
11 checks passed
@anselmbradford anselmbradford deleted the ans_sass branch August 22, 2024 13:07
@chosak chosak mentioned this pull request Sep 25, 2024
5 tasks
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.

3 participants