-
Notifications
You must be signed in to change notification settings - Fork 9
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: Update admin page titles #2410
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
{% block title %} | ||
Log in | Cal-ITP Benefits Administrator | ||
{% endblock title %} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary here, because the {% extends "admin/login.html" %}
default Django login page's title uses Log in | site_title
.
{% block title %} | ||
Logged out | Cal-ITP Benefits Administrator | ||
Logged out | {{ site_title }} | ||
{% endblock title %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logged_out
page extends from agency-base
, and not the default Django admin log out template, so it's necessary to explicitly spell out Logged out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can defer this suggestion)
Since we're now setting site_title
, etc. on BenefitsAdminSite
, I wonder if it'd make sense to make use of the title logic in base_site.html
:
{% block title %}{% if subtitle %}{{ subtitle }} | {% endif %}{{ title }} | {{ site_title|default:_('Django site admin') }}{% endblock %}
The idea is we would make agency-base.html
extend base_site.html
instead of base.html
. We'd then set subtitle
and title
in the context
dict, which better aligns with how the default Django admin app configures its page titles, e.g. login
.
The way you've implemented setting page titles in this PR works, and I know we're trying to finish this out soon, so maybe we defer using this approach for the future, if ever.
This looks good! I like how the titles are organized now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 requested change about success / error page titles. The other 2 comments are just notes
site_title = "Cal-ITP Benefits Administrator" | ||
site_header = "Administrator" | ||
index_title = "Dashboard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to note here: @lalver1 and I had thought about setting these back when he was working on #2313; we chose not to because we didn't want to affect the superuser view.
However, now that we have the spec from product for the page titles and can see more places that need this copy, I think it makes sense to go ahead and set/use these attributes.
So the note here is that the superuser view will now use this copy as well, and I think that is perfectly fine. (We'll probably make the superuser UI and transit agency staff UI look more similar at some point anyways.)
dev-benefits | this branch |
---|---|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for documenting this!
{% block title %} | ||
Logged out | Cal-ITP Benefits Administrator | ||
Logged out | {{ site_title }} | ||
{% endblock title %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can defer this suggestion)
Since we're now setting site_title
, etc. on BenefitsAdminSite
, I wonder if it'd make sense to make use of the title logic in base_site.html
:
{% block title %}{% if subtitle %}{{ subtitle }} | {% endif %}{{ title }} | {{ site_title|default:_('Django site admin') }}{% endblock %}
The idea is we would make agency-base.html
extend base_site.html
instead of base.html
. We'd then set subtitle
and title
in the context
dict, which better aligns with how the default Django admin app configures its page titles, e.g. login
.
The way you've implemented setting page titles in this PR works, and I know we're trying to finish this out soon, so maybe we defer using this approach for the future, if ever.
7d896e5
to
becff48
Compare
@angela-tran @lalver1 Thanks for the review. I wanted to do the simplest fix for now, as it's the last day of the sprint. But we could make a ticket in the future to refactor like @angela-tran suggested above at a later time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - thank you @machikoyasuda !
closes #2381
What this PR does