Cross-repository style cleanup

randimays commented 3 months ago

Status

[2024-08-05] [Fran] In Ready because this has recently caused PROD issues.

Description

Back when vets-website was split into two repositories (creating content-build), the CSS classes/styles that content-build used were not duplicated into the content-build repository. Today, content-build still references some style declarations in vets-website. This has proved brittle as we upgrade to the design system's web components and as the design system team makes changes to their repositories.

A prod issue that occurred recently due to this dependency; here's a PR fix for context: https://github.com/department-of-veterans-affairs/content-build/pull/2140

Micah from the design team compiled a spreadsheet of classes used in content-build that should be audited: https://docs.google.com/spreadsheets/d/1bkdjXasWkgSHCeTPMpUyex7FunUKjDDr1Uj3FIr_M6w/edit?gid=0#gid=0

Slack threads for context
- https://dsva.slack.com/archives/C06V7AAFVH7/p1722024302075569
- https://dsva.slack.com/archives/C52CL1PKQ/p1718906568437409
Recent postmortem of prod issue that occurred due to this dependency: https://github.com/department-of-veterans-affairs/va.gov-team-sensitive/blob/master/Postmortems/2024/2024-06-21%20-%20distorted%20icons%20on%20hub%20pages.md

User story

AS A Veteran, their caregiver, family member or survivor I WANT VA.gov pages to be stable and not display style issues SO THAT I'm not frustrated or confused by what I'm viewing or unable to view.

AS A PO/PM for products in vets-website I WANT VA.gov pages to be stable, not cause prod issues, and not display style issues SO THAT Veterans, their caregivers, family members and survivors are not frustrated or confused by what they're viewing or unable to view.

Engineering notes / background

Refer to Micah's spreadsheet and:

remove classes that are unused in content-build (remember to check JS behavior for usage)
if a class is used in content-build but the style block is only in vets-website, duplicate the style block into content-build
- if the style block in vets-website is not used in vets-website, remove the style block in vets-website

Once everything on the spreadsheet is addressed, we should perform another audit to be sure nothing was missed.

Quality / testing notes

[ ] Test the styles on every template that was changed
[ ] Update any unit tests affected
[ ] Update any e2e tests affected
[ ] If any style updates change the design of the component, get design review

Acceptance criteria

[ ] Review and take appropriate action for all items in Micah's spreadsheet
[ ] Create documentation to share with AP for them to review any implications to the Next Build templates.
[ ] a11y review

aklausmeier commented 3 months ago

@FranECross @jilladams This work should likely include documentation to share with AP for them to review any implications to the Next Build templates.

laflannery commented 3 months ago

Summary of what was brought up in scrum:

AP has not currently moved/migrated any of the existing styles that are explicitly declared in content-build
When they have noticed that what they are implementing differs from what is on Prod and the reason is because of these specific content-build styles, they have asked us (typically Amanda or Laura) for clarification on what should be done. Should updates be made to align the element with the DS or are the custom styles actually necessary and do they need to be pulled over?
So far, very few instances of this have occurred and we have always decided on going with default DS styles and so far we have found no need to pull any content-build custom styles or classes into next-build
We want to make sure that AP is kept in mind and aware because if we find that there are cases where the styles in content-build cannot be replicated or replaced with DS styles or classes, that is a case where would most likely need to keep the existing content-build styles and ultimately move them into next-build

@jilladams @randimays @aklausmeier Let me know if the above makes sense or I missed anything you heard me say in scrum that should be documented

jilladams commented 3 months ago

@randimays I reread #18365 and am pretty sure it's a straight dupe of this ticket now -- thoughts? If you agree, I'll close it.

randimays commented 3 months ago

@jilladams Yep, agree!

jilladams commented 1 month ago

EOS: delayed on CI issues, PRs are up.

randimays commented 1 month ago

I was able to get the first chunk of this code merged yesterday after quite some time trying to get CI to pass. I just realized my ticket was in the wrong status this morning so moved it back to in progress. I'm working on the other chunks of code (both CB and VW). I'm double- and triple-checking that things I am moving/changing/deleting are done so without regressions. PRs should be ready for review today.

randimays commented 1 month ago

Per this Slack thread, putting this ticket down for now. The scope is much larger than originally anticipated, and we need to work on higher-priority items.

Spreadsheet (only in Google) PDF: VACMS-18765.pdf

This spreadsheet shows the class names that were identified as reaching across vets-website and content-build. There is a Status column indicating whether changes for that class name have been merged. The ones that do not have "Merged" are still in a WIP PR.

"Merged" class names PR: https://github.com/department-of-veterans-affairs/content-build/pull/2270

WIP PRs:

Putting this ticket back in the Backlog.

jilladams commented 1 month ago

@randimays could you also make sure that spreadsheet is in the Team folder? It can go to this folder: https://drive.google.com/drive/u/1/folders/1JgRbbEXhZog3YoqrM1T84NUFNJsLpFuk

On Thu, Sep 19, 2024 at 10:43 AM Randi Mays @.***> wrote:

Per this Slack thread https://dsva.slack.com/archives/C52CL1PKQ/p1726766748743269, putting this ticket down for now. The scope is much larger than originally anticipated, and we need to work on higher-priority items.

Spreadsheet https://docs.google.com/spreadsheets/d/1fsrAN3VFHASZ34UvTCBbvBoPF9FPda-YB07NOZ5nZyM/edit?gid=0#gid=0 (only in Google) PDF: VACMS-18765.pdf https://github.com/user-attachments/files/17063698/VACMS-18765.pdf

This spreadsheet shows the class names that were identified as reaching across vets-website and content-build. There is a Status column indicating whether changes for that class name have been merged. The ones that do not have "Merged" are still in a WIP PR.

"Merged" class names PR: department-of-veterans-affairs/content-build#2270 https://github.com/department-of-veterans-affairs/content-build/pull/2270

WIP PRs:

department-of-veterans-affairs/vets-website#31856 https://github.com/department-of-veterans-affairs/vets-website/pull/31856

department-of-veterans-affairs/content-build#2267 https://github.com/department-of-veterans-affairs/content-build/pull/2267

Putting this ticket back in the Backlog.

— Reply to this email directly, view it on GitHub https://github.com/department-of-veterans-affairs/va.gov-cms/issues/18765#issuecomment-2361813595, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUM55H45TD63ROKB3TTYMH3ZXMELNAVCNFSM6AAAAABLUTMDCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRRHAYTGNJZGU . You are receiving this because you were mentioned.Message ID: @.*** com>

randimays commented 1 month ago

@jilladams Done!

department-of-veterans-affairs / va.gov-cms