department-of-veterans-affairs / vets-design-system-documentation

Repository for design.va.gov website
https://design.va.gov
37 stars 57 forks source link

CSS Library - Utilities - Swap background-color and color imports #3060

Open micahchiang opened 1 month ago

micahchiang commented 1 month ago

Description

vets-website currently still imports all utility stylesheets from Formation instead of css-library. This is the way it's done:

  1. All utilities are imported into Formation's core.scss file
  2. core.scss is imported into vets-website's style.scss file.

To switch imports from Formation to css-library, we'll first import the css-library utilities.css file into vets-website's style.scss below the import for Formation's core.scss. Then we'll turn off the corresponding utility import in Formation by commenting it out of core.scss. By doing this, we get around the problem of all utility classes in Formation using !important, and we just roll out Formation releases that include the commented out imports.

This ticket is to swap out background-color and color utilities.

Considerations

Tasks

Acceptance Criteria

caw310 commented 1 month ago

Hey team! Please add your planning poker estimate with Zenhub @Andrew565 @ataker @harshil1793 @it-harrison @jamigibbs @micahchiang @powellkerry @rmessina1010 @rsmithadhoc

rmessina1010 commented 3 weeks ago

I will need to carry this over to next sprint. The E2E Errors seem to be the result of color inconsistencies, that have to be looked at individually.

rmessina1010 commented 1 week ago

SUMMARY

The ask

The original task was to switch the names of color utility classes, specifically those having to do with vads-u-... primary color as applied to color and background properties. Also to bump up css-library to the corresponding version. Since this was just a name change, at completion, the expected results was for va.gov to remain exactly as before with no visual or function changes, nor emerging test errors. By itself this task was very systematic and straight forward.

Finding errors

Testing however did reveal multiple cascading test failures that needed to be resolved due to the changes in css library. While there were other issues, the one most germane to this PR resulted from the removal of the !important keyword from the main.scss style sheet. these lead to the utility classes not applying at all or being overridden all depending on ALL of the OTHER classes and rules.

It was fortunate at this PR was so narrow in focus as it allowed me to do the following:

First, to make sure that no other changes to css library were affecting this PR, I commented out the file's import and inserted instead just the corresponding color rules at the exact location of the stylesheet import.

Make a list of the specific apps and even element/component tags to which the new classnames were applied.

A challenge in fixing the errors was that while some errors were glaring, others were visible only at specific steps of a user view of an application, and thus not readily seen or reproduced. for this reason I relied on VSF teams as well as Cypress Test's screen shots to get an idea of what was amiss for each instance.

CONCLUSION

Causes and Fixes

It seems that removing the !important keyword from utility classes, especially those defined in the main.scss leaves them subject to regular CSS behavior for import order and specificity.

So for example, given the following files with the described rules, and corresponding usage markup:

main.scss
.vads-u-color-pruple { color: purple;}

b_buttons.scss
.va-button-primary { color: reflex-blue;}

someComponent.jsx
<button  className="va-button-primary vads-u-color-pruple additonal-class" >OHAI!!</button>

In the above case, the button text will be in reflex-blue and not the purple expected. This is because _buttons.scss gets imported after main.scss and since both classes address the color field the last import takes precedence [fun fact: we use this VERY CONCEPT as a the basis for all our override stylesheets, this is why they have to be imported last]

This is complicated further more when you have multiple classes involved in the stylesheet:

m_additional.scss
button.va-button-primary.additonal-class { color: red;}

In this, rhetorical, case (that could surface from platform or vfs customization), the button text color will be red ...not the expected purple... or blue, since the rule includes two classes and an element, so would need a rule with at least equal specificity to override it.

Almost all classNames in va.gov are composed that is tags and components have a litany of classNames both utility and not that may have conflicting functions. For this PR I had to: 1) find the visual error 2) check it against every instance of a tag whose class name I had modified 3) inspect the rules and files applied ( tho I found I could anticipate that some module defaults was the a frequent but not sole culprit) 4) inspect the source code of the app in browser for a structure to help me select classes to build my custom rule. This is again challenging as many of the instances in question did not include any "structural" classes or idsin them-- and you DO NOT want to be generic when making an override rule. Sometimes you will have to go several ancestors up to find what you need. for example...for button.va-button-primary.additonal-class{..} you may need to find something like #myApp so you can do#myApp vads-u-color-pruple{..} or you may need to find .my-app-header and another ancestor .my-app then do.my-app .my-app-header button.vads-u-color-pruple{..} 5) review to see if any of these new rules can be consolidated 6) finally place this new rule in the app's main style sheet or if thats not an option then use shame.scss

This has to be done for every instance changed.

Caveats

While this PR fixes current bugs. It cannot address the root cause of the issue. If new instances of this classes are added, they may just as well break and need their own custom fixes as described above.

THINGS TO CONSIDER

Can a utility guarantee its function?

If the expected result of adding a utility class is to definitively change a specific field value of the element to which it is applied, I think we should reconsider the removal of the !important keyword as this is the most efficient way of achieving that result.

We should also re-consider the order of import of utilities,

relative to modules and other such stylesheets.

Reconsider module construction

Some of the aforementioned conflicts happened because the rules defining the appearance of a button included a defined default color. as such you had a conflict between the css module for buttons and the utility classes style sheets. The opposite approach, of building classes for each permutation could be viable to more labor intensive

Handling and edge cases

(such as multi class rules) will also require review of style sheets or depend on VFS team support

caw310 commented 1 week ago

This will carry over as Ray is out and has an open PR for this.