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

Formation Global Updates: Update base font size and convert rem value #987

Merged
merged 30 commits into from Mar 27, 2024

Conversation

jamigibbs
Copy link
Contributor

@jamigibbs jamigibbs commented Nov 7, 2023

Using Verdaccio to publish this package locally, we can review how these changes look with vets-website.

Description

This PR updates Formation so that it uses the USWDS v3 root font-size which are:
html: 100%
body: 16px

It adds a custom REM conversion function so that we can update the old Formation REM values to the USWDS v3 rem value.

I've started to go through each of the stylesheets and either:

a) wrapped any rem values with a conversion function, or,
b) created a duplicate of an external libraries stylesheet (this is mostly the USWDS v1 imports), added it to the /formation-overrides/ folder, and wrapped any rem values with the conversion function. Then just replaced the old import with the new one. For example.

Any place that I have added a // [x] note means that the stylesheet has been reviewed and updated (if necessary):

https://github.com/department-of-veterans-affairs/veteran-facing-services-tools/blob/07305e2d2b4815c98b21be8c23f0bf8f2677b029/packages/formation/sass/core.scss

Note: If a duplicate stylesheet needed to be created for a uswds v1 import, that needs to be added to the minimal stylesheet as well. Here is the related PR for the minimal stylesheet: department-of-veterans-affairs/vets-website#26606

Testing done

Screenshots

Acceptance criteria

  • [ ]

Definition of done

  • Changes have been tested in vets-website
  • Changes have been tested in IE11, if applicable
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

Signed-off-by: Micah Chiang <micahkchiang@gmail.com>
Signed-off-by: Micah Chiang <micahkchiang@gmail.com>
Signed-off-by: Micah Chiang <micahkchiang@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

$text-max-width:      70rem; // Note: USWDS value is 53rem;
$site-max-width:      100rem; // Worksout to about 1000px. USWDS value is 1040px

Here it looks like USWDS deviates from the coefficient ( 10/16 = 0.625). This is an example of a possible arbitrary change made by USWDS. If we know for a fact that the required value is 53rem, and not 43.75rem (.0625 * 70rem = 43.75rem), then this need to be manually calculated.

(will add. util for handling cases like this) :)

Copy link
Contributor Author

@jamigibbs jamigibbs Feb 1, 2024

Choose a reason for hiding this comment

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

@rmessina1010 We are not trying to match USWDS v3 at this point. We are just trying to convert rems so that they calculate to 16px root font size.

Copy link
Contributor

Choose a reason for hiding this comment

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

on line 32, why not use font-size:$uswds-base ; ?

Copy link
Contributor

Choose a reason for hiding this comment

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

A worthwhile note about the usage of !default, the keyword makes it so that the value is applied only if the variable name has not been previously defined, or is null. This means that if , in the import order, a previous scss document defines any of these variables, it's the value in the said document that will remain. Just checking this is what's desired.

Signed-off-by: Micah Chiang <micahkchiang@gmail.com>
@micahchiang micahchiang force-pushed the 66753-formation-font-size-override branch from 3ecc394 to 1dd84f4 Compare February 5, 2024 23:12
Signed-off-by: Micah Chiang <micahkchiang@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleted. Unused in vets-website, content-build, and doc site.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleted. Unused in vets-website, content-build, and doc site.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleted. Unused in vets-website, content-build, and doc site. We'll have to remove the imports from vets-website. Classes themselves aren't used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Table instances have been migrated to va-table.

}
// if (branchName !== 'master') {
// console.log(
// chalk.yellow(
Copy link
Contributor Author

@jamigibbs jamigibbs Mar 20, 2024

Choose a reason for hiding this comment

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

I don't think this was supposed to be checked in.

@powellkerry powellkerry marked this pull request as ready for review March 25, 2024 19:44
@powellkerry powellkerry requested review from a team as code owners March 25, 2024 19:44
Copy link
Contributor

Choose a reason for hiding this comment

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

line 19 has a typo
it should be max-width: scale-rem(100rem);

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

in padding-left: calc(#{scale-rem(2rem)} - 7px);
was the #{..} necessary? it's not an scss variable, this may introduce errors

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct based on sass documentation and stackoverflow: https://stackoverflow.com/questions/46097871/how-to-use-calc-with-function-inside

When I try to remove that syntax I get an error in vs-code saying that the ending parenthesis was expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on some research, it seems like since scale-rem can return a string (e.g. 1rem), keeping that interpolation is the correct thing to do here

Copy link
Contributor

Choose a reason for hiding this comment

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

padding-left: 2rem - .9375rem;
was this meant to be a calc()?
if so, the conversion should be scale-rem(calc(2rem - .9375rem));

Copy link
Contributor

Choose a reason for hiding this comment

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

I replaced this with padding-left: scale-rem(1.0625rem);

…tment-of-veterans-affairs/veteran-facing-services-tools into 66753-formation-font-size-override
@rmessina1010 rmessina1010 self-requested a review March 27, 2024 15:35
Copy link
Contributor

@rmessina1010 rmessina1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@powellkerry powellkerry merged commit 9fd9ed3 into master Mar 27, 2024
5 of 6 checks passed
@powellkerry powellkerry deleted the 66753-formation-font-size-override branch March 27, 2024 17:05
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.

None yet

6 participants