Opened 4 years ago
Closed 4 years ago
#33667 closed Cleanup/optimization (fixed)
Django admin stylesheet ignores --header-branding-color variable.
| Reported by: | Mike DeSimone | Owned by: | Hrushikesh Vaidya |
|---|---|---|---|
| Component: | contrib.admin | Version: | 3.2 |
| Severity: | Normal | Keywords: | css, stylesheet, admin |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
In contrib/admin/static/admin/css/base.css, a header branding color variable is defined at line 20:
--header-branding-color: var(--accent);
but it is not used anywhere else. I expected it to be used in the branding h1 tags starting at line 920:
#branding h1 {
padding: 0;
margin: 0 20px 0 0;
font-weight: 300;
font-size: 24px;
color: var(--accent);
}
#branding h1, #branding h1 a:link, #branding h1 a:visited {
color: var(--accent);
}
but as can be seen, var(--accent) is used instead. Also the second block redundantly calls out #branding h1. Any idea what it should have been? #branding h1 a?
--header-branding-color appears nowhere else in Django. This bug is found in 3.2, 4.0, and the main branch.
Change History (7)
comment:1 by , 4 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Type: | Bug → Cleanup/optimization |
comment:2 by , 4 years ago
Would the patch get into a 3.2 update? It does me no good if it's only fixed on main.
I might be able to get to it in a week or three; I ran into it on an airgapped system and would need to recreate it on another machine some weekend or another. (I already had to jump through internal hoops just to report this.) I have no problem if someone wants to roll the fix in themselves.
Also, what else would need to be in the patch? I don't know how to generate a test case for this.
comment:3 by , 4 years ago
Would the patch get into a 3.2 update? It does me no good if it's only fixed on
main.
Unfortunately not, it doesn't qualify for a backport.
Also, what else would need to be in the patch? I don't know how to generate a test case for this.
IMO a regression test is not needed.
comment:4 by , 4 years ago
Unfortunately not, it doesn't qualify for a backport.
Then making a patch is low priority for me. I'm going to have to override #branding h1 directly anyway.
Hmm, it seems --accent isn't used anywhere but these two places in #branding. Is this also an oversight? I'm wondering which fix is better:
- use
--header-branding-colorin#branding h1and--accentin#branding h1, #branding h1 a:link, #branding h1 a:visited(changing the first#branding h1to#branding h1 a) - remove
--header-branding-coloras redundant, or - use
--accentelsewhere?
It's hard to say since I'm not the style designer.
comment:5 by , 4 years ago
I would use --header-branding-color for #branding h1 and leave --accent for #branding h1 a:link, #branding h1 a:visited.
comment:6 by , 4 years ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
I've prepared a patch PR
Agreed,
--header-branding-colorshould be used for#branding h1As far as I'm aware,
#branding h1is redundant in the second rule and can be removed. Would you like to prepare a patch?