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.

Sign up for GitHub

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

Jump to bottom

Chore: extract common Admin overrides #2430

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Chore: extract common Admin overrides #2430

merged 4 commits into from
Oct 9, 2024

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Oct 7, 2024 •
edited
Loading

Closes #2425

This PR has a few cleanups. It fixes the indentation of the usertools block in agency-base; extracts favicon, Bootstrap, and styles.css for the admin app; and extracts the login and logged_out branding.

@lalver1 lalver1 self-assigned this Oct 7, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Oct 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024 •
edited
Loading

Coverage report

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

@lalver1
Copy link
Member Author

lalver1 commented Oct 8, 2024

I could group bootstrap.html, favicon.html and style-admin.html into one html file but I was having trouble finding a short name for it and also thought that breaking them out made the {% include %} statements clearer, but I'm leaning either way.

@lalver1 lalver1 marked this pull request as ready for review October 8, 2024 14:42
@lalver1 lalver1 requested a review from a team as a code owner October 8, 2024 14:42
@angela-tran
Copy link
Member

I could group bootstrap.html, favicon.html and style-admin.html into one html file but I was having trouble finding a short name for it and also thought that breaking them out made the {% include %} statements clearer, but I'm leaning either way.

I think it's better to have them as separate includes 👍

machikoyasuda
machikoyasuda reviewed Oct 8, 2024
View reviewed changes
benefits/templates/admin/includes/favicon.html
@@ -0,0 +1,3 @@
{% load i18n static %}

<link href="{% static "img/favicon.ico" %}" rel="icon" type="image/x-icon" />
Copy link
Member

@machikoyasuda machikoyasuda Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe need a future ticket: The favicon doesn't show up on the superuser homepage, but it does for the other admin pages: log in, log out, in-person flow pages.

image

cc @indexing - Just to confirm, do we want Admin to have the same favicon as Benefits (digital flow)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @machikoyasuda! I'm looking into this to see where favicon gets set in the index admin page that we see when we log in as a superuser.

Copy link
Member

Choose a reason for hiding this comment

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

Yes @machikoyasuda. Let's use the same favicon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! I'll create a separate ticket to fix this favicon issue.

@lalver1 lalver1 force-pushed the chore/common-admin-overrides branch from 79c47ae to 6ee7e3c Compare October 8, 2024 18:35
@lalver1 lalver1 force-pushed the chore/common-admin-overrides branch from 6ee7e3c to 4cbc4a2 Compare October 9, 2024 14:11
angela-tran
angela-tran approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@angela-tran angela-tran 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 good to me. Thanks!

@lalver1 lalver1 merged commit 889e4d8 into main Oct 9, 2024
10 checks passed
@lalver1 lalver1 deleted the chore/common-admin-overrides branch October 9, 2024 15:22
@lalver1 lalver1 mentioned this pull request Oct 9, 2024
Closed
@lalver1 lalver1 mentioned this pull request Oct 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indexing indexing indexing left review comments

@machikoyasuda machikoyasuda machikoyasuda left review comments

@angela-tran angela-tran angela-tran approved these changes

Assignees

@lalver1 lalver1

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extract out the common Admin overrides
4 participants
@lalver1 @angela-tran @machikoyasuda @indexing