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

css-library: core imports #1359

Draft
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

micahchiang
Copy link
Contributor

@micahchiang micahchiang commented Oct 8, 2024

Chromatic

https://mc-update-css-library-core-stylesheet--65a6e2ed2314f7b8f98609d8.chromatic.com


Configuring this pull request

  • Link to any related issues in the description so they can be cross-referenced.
  • Add the appropriate version patch label (major, minor, patch, or ignore-for-release).
    • See How to choose a version number for guidance.
    • Use ignore-for-release if files haven't been changed in a component library package. (ie. only Storybook files)
  • DST Only: Increment the /packages/core version number if this will be the last PR merged before a release.
  • Complete all sections below.
  • Delete this section once complete

Description

This work updates css-library/stylesheets/core.scss with the necessary changes to make things work in vets-website

QA Checklist

  • Component maintains 1:1 parity with design mocks
  • Text is consistent with what's been provided in the mocks
  • Component behaves as expected across breakpoints
  • Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
  • Designer has signed off on changes (if applicable. If not applicable provide reason why)
  • Tab order and focus state work as expected
  • Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
  • New components are covered by e2e tests; updates to existing components are covered by existing test suite
  • Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

Acceptance criteria

  • QA checklist has been completed
  • Screenshots have been attached that cover desktop and mobile screens

Definition of done

  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

Signed-off-by: Micah Chiang <[email protected]>
Signed-off-by: Micah Chiang <[email protected]>
Copy link
Contributor Author

@micahchiang micahchiang left a comment

Choose a reason for hiding this comment

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

Leaving comments on changes here as I make them

@@ -1,6 +1,6 @@
{
"name": "@department-of-veterans-affairs/css-library",
"version": "0.12.2",
"version": "0.12.2-rc2",
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'll update this whenever this PR is ready to be merged.

Comment on lines 39 to 40
@import 'base/fonts';
@import 'base/utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fonts is needed because it's the entry point for uswds-fonts, which contains all of the font-face settings for source sans pro web. Prior to this, even though we were using uswds-typography settings, it seems like our imports for fonts in vets-website were still relying on formation/core.

utils is needed because it contains at least one style rule, va-button-link that is actively being used in vets-website, but we should definitely look to deprecate this moving forward.

Comment on lines +4 to +9
"sans": { "value": "'Source Sans Pro Web', 'Helvetica Neue', Helvetica, Roboto, Arial, sans-serif" },
"serif": { "value": "Bitter, Georgia, Cambria, 'Times New Roman', Times, serif" }
},
"serif": { "value": "Bitter, Georgia, Cambria, 'Times New Roman', Times, serif" },
"source": {
"sans": { "value": "'Source Sans Pro', 'Helvetica Neue', Helvetica, Roboto, Arial, sans-serif"}
"sans": { "value": "'Source Sans Pro Web', 'Helvetica Neue', Helvetica, Roboto, Arial, sans-serif"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

our utility classes needed to be updated with the newly imported base/fonts because of the uswds rename to Source Sans Pro Web. Without this addition the utility class vads-u-font-family--sans will render a serif font due to not finding the right font-face.

Signed-off-by: Micah Chiang <[email protected]>
@@ -1,6 +1,6 @@
{
"name": "@department-of-veterans-affairs/css-library",
"version": "0.12.2",
"version": "0.12.2-rc6",
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'll update this with a proper version when its ready

Comment on lines 19 to 21
@import 'base/va';
@import 'base/fonts';
@import 'base/utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these higher because we likely want any formation override stylesheets to take precedent

@@ -87,7 +87,6 @@ h6 {
font-family: tok.$font-family-serif;
line-height: vars.$heading-line-height;
margin-bottom: 0.5em;
margin-top: 1.5em;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this because it usually ends up being overridden in applications anyway.

In general, we should move to remove this file completely and instead use base/headings.scss because that uses uswds heading styles. The main thing in this rule that is still needed in vets-website is the margin-bottom rule, which several applications expect by default.

@@ -106,7 +106,7 @@ a {
}

// ====== Headings
@import 'headings';
// @import 'headings';
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'm temporarily removing this because it contains v3 heading styles which are colliding with Formation heading styles when attempting to swap core imports in vets-website.

headings.scss imports uswds v3 usa-headings-styles, which includes some things we may or may not want. For example, this typeset-heading that adds margin top with a universal selector, and uses theme paragraph margin settings.

I attempted to reconcile these with the existing heading styles, but felt like I was beginning to change too many things at once.

We definitely want to get rid of the stuff in the formation-overrides folder, but we need a more systematic approach for this.

Comment on lines +52 to +53
$focus-outline: 2px solid $vads-color-action-focus-on-light;
$focus-spacing: 2px;
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 fixes focus outline for anchor tags that are not contained in one of our web components. A lot of these anchor tags tend to live on hub pages somewhere. This mirrors the web components mixin.

Eventually, we'll want to probably have css-library be the source of truth for mixins, and instead inherit web-component mixins from there.

@@ -348,6 +355,7 @@ article > h1 {
max-width: $lead-max-width;

p, a {
font-family: $font-serif;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

va-introtext p is used on a bunch of different hub pages. Without explicitly setting font-family to $font-serif these paragraphs will render source sans pro instead of bitter which is how they're currently rendered. This is because our uswds-typography stylesheet will override font-family.

Comment on lines +280 to +284
font-size: 12px;
padding-left: scale-rem(1.5rem);
p {
font-size: 12px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to these styles are needed to maintain parity with what's currently in production. Leaving font-size out here will render text to 16px.

Signed-off-by: Micah Chiang <[email protected]>
Comment on lines +30 to +33
h1 {
margin-top: 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed for home page and hub pages mostly

Comment on lines 370 to 405
// Used on content-build hub pages and a couple of places in vets-website
.va-h-ruled--stars {
display: flex;
justify-content: center;
align-items: center;

&::before {
margin-right: scale-rem(12rem);
}

&::before,
&::after {
border-top: 1px solid $color-gray-light;
content: " ";
flex: 1 1 50%;
padding: scale-rule(1rem 0);
}

background: url('#{$image-path}/stars.png') no-repeat center;
background-size: scale-rule(11rem auto);
margin: scale-rule(1.6rem auto auto);
padding: scale-rule(2rem 0 0);
text-align: center;

@include media($medium-screen) {
padding-left: 0;
}
}
// used mostly in content-build, we should consider moving this over there
.last-updated {
margin-top: 1.5em;
border-top: 2px solid $color-gray-light;
padding: 1em 0;
p {
color: $color-gray-dark !important;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These render the spacers on static hub pages that include the stars png. Without them an empty space is rendered instead.

The last-updated class renders the top border. Without it and empty space is rendered instead.

Signed-off-by: Micah Chiang <[email protected]>
Comment on lines +15 to +16
// This actually seems to be how things were before.
// Leaving a comment here so it gets flagged in the PR.
Copy link
Contributor Author

@micahchiang micahchiang Oct 10, 2024

Choose a reason for hiding this comment

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

Specifically calling this out because it renders a larger margin top than what's currently on va.gov, but I think this is actually correct and more akin to what was formerly on va.gov before we began our work to scale everything for root font-size migration.

The original rule is here and it uses the scale-rule function we created. However, it turns out that currently throws an error in production dev tools and falls back to a reduced margin:

image

Contrast the above with the margin-top rule correctly resolving when testing locally with the css-library/core file:

image

I tried confirming this with wayback machine...but uh they got ddos'd today.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to load this is archive.gov now for a date last year and you're right that there was originally more margin but it still looks different than your second screenshot.

The calculated value is 26.8px from that time.

I think if we remove scale-rule and convert 3rem to 1.875rem, it will be correct.

Screenshot 2024-10-21 at 2 25 32 PM Screenshot 2024-10-21 at 2 27 23 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, yeah I wasn't 100 percent sure what the correct margin was. Thanks for confirming.

@@ -17,7 +17,7 @@
}

.usa-alert-heading {
font-family: "Source Sans Pro";
font-family: $font-family-sans;
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 file is already importing the variables file, and $font-farmily-sans contains the updated Source Sans Pro Web naming convention. Renaming this fixes rendered font.

@@ -17,7 +17,7 @@
}

.usa-alert-heading {
font-family: "Source Sans Pro";
font-family: $font-family-sans;
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 file is already importing the variables file, and $font-farmily-sans contains the updated Source Sans Pro Web naming convention. Renaming this fixes rendered font.

@@ -103,7 +103,7 @@ $marketing-container-height: 380px;
h3 {
display: block;
color: $vads-color-black;
font-family: Source Sans Pro, sans serif;
font-family: $font-family-sans;
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 file is already importing the variables file, and $font-farmily-sans contains the updated Source Sans Pro Web naming convention. Renaming this fixes rendered font.

@@ -260,7 +260,7 @@ $marketing-container-height: 380px;

.vetnav-panel--submenu:not([hidden]) {
h3 {
font-family: Source Sans Pro, sans serif;
font-family: $font-family-sans;
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 file is already importing the variables file, and $font-farmily-sans contains the updated Source Sans Pro Web naming convention. Renaming this fixes rendered font.

@use './mixins' as *;

//============ VENDOR PARTIALS ==============//
@import 'lib/normalize';

// Grid/columns classes
@import "./formation-overrides/foundation-sites";
Copy link
Contributor

@jamigibbs jamigibbs Oct 23, 2024

Choose a reason for hiding this comment

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

This should handle being a bridge for the Foundation imports being imported into Formation's core stylesheet here since these classes are still widely used in vets-website. Thank you to Kerry for getting them out of the Foundation package! 🙇🏼‍♀️

jamigibbs and others added 22 commits October 24, 2024 09:58
Signed-off-by: Micah Chiang <[email protected]>
Signed-off-by: Micah Chiang <[email protected]>
Signed-off-by: Micah Chiang <[email protected]>
@@ -11,11 +11,12 @@
@use '../../override-function' as *;
@use '../../mixins' as fm;

$usa-form-width: scale-rem(32rem);

$usa-form-width: 32rem;
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 think this value might have been updated unnecessarily. In formation, $usa-form-width is overridden in this stylesheet, which is imported after all other imported stylesheets that involve form width.

Here's a diff between local and staging that illustrates the side effect:
image

And here's a diff with the scale-rem rule removed:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had the below override styles in formation, and they seem to align more with what's currently in production for forms. So bringing those over instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants