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

Admin: Header theme, remove darkmode toggle & breadcrumb theme #2773

Merged
merged 30 commits into from
Mar 27, 2025

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Mar 20, 2025

closes #2738
closes #2739
closes #2742

What this PR does, in order

  • Delete unnecessary templates
  • Removes dark mode, in 1 spot only: in base.html
  • Favicon, Branding (logo icons), Bootstrap CSS, Styles (old code), Theme (new code), Analytics and are declared in 1 spot: in base.html
  • Universal header: All admin pages have the same header. The only new HTML necessary to make this happen is branding.html. It is used on all pages, including login/log out, agency flows and Django admin pages, to add the logo images to the header. The rest of the header is the default.
  • Breadcrumb colors added to theme
  • How overriding works, as demonstrated with branding.html across 2 different template styles: The theme.css file uses a body class, CSS variables to add per-page/per-template styling. For example, although all pages have the logo image, the login/log out page's logo size is different. That is declared in the theme.css file.

Notes

The goal of this overall effort is to have:

  1. A theme.css file that gives the admin the look of the header that has been designed on Figma.
  2. Demonstrate how to write the theme.css file in various scenarios.
  3. Avoid writing HTML for Django header overrides like Admin: Header and Theme Toggle #2766 did.

This PR does not achieve:

  1. Pixel perfect comps from Figma, but gets pretty close within constraints (like not changing to Noto yet, not touching button CSS).
  2. Some things are impossible to change - like, there is no way to add spacing after the Welcome, username. There is no way to remove the View site link (in a clean way - there are hacky ways to remove this with CSS, probably).

The way I'd like to move forward after the primitives Design work (#2648) on Admin is done, is to slowly move code from styles.css into theme.css.

Page-specific changes

Logged out

image

  • Entirely refactors logged_out template to be a lot simpler.
  • logged_out template now inherits from login.html.

Log in / Logged out

image

  • Adds border

Django admin

image
image

  • Adds universal header.
  • Fixes any weird Bootstrap regressions

In-person flow

image
image

  • Adds universal header
  • Adds a few Bootstrap font utility classes to get the title font sizes right.
  • Future work necessary to get buttons sized right.

Documentation of best practices

@machikoyasuda machikoyasuda self-assigned this Mar 20, 2025
@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 labels Mar 20, 2025
Copy link

github-actions bot commented Mar 20, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

@machikoyasuda machikoyasuda changed the title Big WIP Admin: Header and theme toggle Mar 20, 2025
@machikoyasuda machikoyasuda changed the title Admin: Header and theme toggle Admin: Header theme and remove darkmode toggle Mar 20, 2025
@machikoyasuda machikoyasuda marked this pull request as ready for review March 21, 2025 00:06
@machikoyasuda machikoyasuda requested a review from a team as a code owner March 21, 2025 00:06
@machikoyasuda
Copy link
Member Author

@thekaveman New PR is up, based on what we discussed today.

Started drafting but did not complete a README.md. I wanted to see how this PR lands first with everyone.

Comment on lines 1 to 5
{% extends "admin/base_site.html" %}
{% extends "admin/base.html" %}

{% block extrahead %}
{% include "admin/includes/favicon.html" %}
{% endblock extrahead %}
Copy link
Member Author

Choose a reason for hiding this comment

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

Originally had intended to delete this file entirely, but all of the Admin app won't get this new branding unless we override this here as well.

Copy link
Member

@thekaveman thekaveman Mar 21, 2025

Choose a reason for hiding this comment

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

I guess I don't fully understand the difference between admin/base.html and admin/base_site.html and why we need both? But I can leave that aside for the moment.

My main question is, if admin/base_site.html (this template) extends admin/base.html (our custom version of that template) -- then why do we need the same {% block branding %} here?

I.e. maybe it is enough to just have this template {% extends "admin/base.html" %} and that's all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try this!

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting the base_site.html file entirely leads to this:
image

Keeping base_site.html but taking out the branding override behaves as expected. The branding override inherits from our base.html.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thekaveman Fixed here: 1dca839

Comment on lines 5 to 17
--primary: #ffffff;
--bs-body-font-size: 1rem;
--bs-body-color: #212121; /* Body text color */
--bs-dark-rgb: 33, 33, 33; /* Background color for Header */
--bs-heading-color: #212121; /* Header text color */
--bs-secondary-bg-subtle: #f1f1f1; /* Background color for Admin, lighter than bs-secondary, used with bg-secondary-subtle */
--bs-secondary-rgb:
210, 210, 210; /* Border color for Admin, used with border-secondary */
--bs-secondary: #c5c4c4;
}
Copy link
Member Author

@machikoyasuda machikoyasuda Mar 21, 2025

Choose a reason for hiding this comment

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

I am leaving this as is, but wanted to note that I'm still noodling with how to format this better overall. In the future, I may explore separating out the Bootstrap variables from Django, etc. It's all still a little overwhelming to have all the variables from variables.css and these hanging over our heads on this app. Hard to have a mental model about it. This will be improved in future refactors once #2648 is done.

/* Login page */

.login .main {
border: 1px solid #000000;
Copy link
Member Author

Choose a reason for hiding this comment

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

Opted not to use a variable here, b/c it's not being repeated everywhere. Again, will be improved when #2648 is done.

@machikoyasuda machikoyasuda force-pushed the feat/2739-admin--theme-refactor branch from f17aba6 to 51604a0 Compare March 25, 2025 17:44
@machikoyasuda machikoyasuda changed the title Admin: Header theme and remove darkmode toggle Admin: Header theme, remove darkmode toggle & breadcrumb theme Mar 25, 2025
@machikoyasuda
Copy link
Member Author

Known small visual things (not quite bugs), but can be fixed in future tickets during Slush:

  • Hovering over the Log out button moves the text up oe pixel image Appears on both sides of admin.

  • Button font size is too big. It is 20px (which is what it is on Benefits), but should be 16px as per Figma.

image

I can create tickets for these as soon as this PR is merged.

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.

This looks great. We're getting closer to a true "admin theme".

@machikoyasuda machikoyasuda merged commit e02c4f2 into main Mar 27, 2025
9 checks passed
@machikoyasuda machikoyasuda deleted the feat/2739-admin--theme-refactor branch March 27, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
2 participants