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

Refactor: Remove CA State Web Template #2526

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Nov 19, 2024

closes #2491

What this PR does

  • Remove CA State Web Template CSS, which will then remove the call to the CA State Web Template font
  • Remove CA State Web Template domain from CSP
  • Makes any CSS fixes necessary

Changelog

  • fix:text-left to text-start
  • fix: add css to fix skip nav
  • fix: button focuses
  • slight refactor: buttons in general. could be greatly refactored to mostly be written with css variables instead, reducing the number of new classes/modified classes.

QA

  • Trigger the skip nav
  • Go through the whole app via keyboard, paying attention to the way the modals behave with the radio buttons
  • Manually enable the JavaScript/Cookies warnings via HTML (remove the no display class) and compare it visually with the current one
  • Check the app on responsive widths

@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates back-end Django views, sessions, middleware, models, migrations etc. labels Nov 19, 2024
Copy link

github-actions bot commented Nov 19, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py
Project Total  

This report was generated by python-coverage-comment-action

@machikoyasuda machikoyasuda force-pushed the refactor/2491-remove-ca-state-web-template branch from 4afee3b to a8a89dd Compare November 25, 2024 19:06
@machikoyasuda machikoyasuda marked this pull request as ready for review November 25, 2024 19:06
@machikoyasuda machikoyasuda requested a review from a team as a code owner November 25, 2024 19:06
@thekaveman
Copy link
Member

fix:text-left to text-start

Was this a CA State Web Template class, and now we use the Bootstrap version or?

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

I'm noticing some slight differences between this branch and prod -- not necessarily a bad thing, just wondering if you have any more insight:

Homepage text is better aligned
This branch prod
992px image image
1200px image image
Overall container/spacing is better on larger screen sizes
This branch prod
1280px image image
1280px image image

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Nov 26, 2024

fix:text-left to text-start

Was this a CA State Web Template class, and now we use the Bootstrap version or?

In general: In short, yes. I believe the CA State Web Template had copied and pasted lots of Bootstrap utility classes but modified some of them. By using the new start/end syntax, we're now on the updated modern version.

In length - It's actually more of a CSS change. text-align: start works now. Check it out here: https://developer.mozilla.org/en-US/docs/Web/CSS/text-align

Using start/end instead of left/right is a way for CSS to be more "content-aware" (is the term I've seen used) which in this context means that if the language of the HTML document is a Right to Left language, the text-align: start woould set text-align: right, and on Left to Right language, text-align: start would set text-align: left. And so when CSS changed to allow this, Bootstrap changed the naming of its classes appropriately. The Bootstrap docs call these changes "logical" descriptions vs. directional for RTL support:

https://getbootstrap.com/docs/5.0/migration/#rtl

https://getbootstrap.com/docs/5.0/migration/#utilities

@machikoyasuda
Copy link
Member Author

@thekaveman Your width change noticings also reflect how CA State Web Template had some different numbers for container max-widths, and now we're on Bootstrap's defaults.

@thekaveman
Copy link
Member

  • Triggered skip nav ✅
  • Went through the app with keyboard only ✅
  • Manually enabled JavaScript/cookie warnings (the text on these is still not contrasted enough) ✅
  • Checked the app on various responsive widths ✅

@machikoyasuda machikoyasuda merged commit ef4d4e8 into main Nov 27, 2024
15 checks passed
@machikoyasuda machikoyasuda deleted the refactor/2491-remove-ca-state-web-template branch November 27, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web perf: Remove CA State Web Template
2 participants