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

Fixed #35505 -- Added extrabody block to admin/base.html. #18251

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

stefanivic
Copy link
Contributor

@stefanivic stefanivic commented Jun 8, 2024

Trac ticket number

ticket-35505

Branch description

The following change introduces a new template block inside the admin/base.html, just before the closing </body> tag that can be used to add any code after the footer section, like custom <script> tags.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Member

@knyghty knyghty left a comment

Choose a reason for hiding this comment

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

Thanks, looking good.

A few things I can immediately think of.

docs/releases/5.1.txt Outdated Show resolved Hide resolved
docs/releases/5.1.txt Outdated Show resolved Hide resolved
@knyghty
Copy link
Member

knyghty commented Jun 8, 2024

We should also add a note into the admin documentation that this block exists and how to override it.

@stefanivic
Copy link
Contributor Author

We should also add a note into the admin documentation that this block exists and how to override it.

Do you have any tips where we should put this ? I mostly see template tags in the docs, but I can't seem to find anything about the blocks.

@knyghty
Copy link
Member

knyghty commented Jun 8, 2024

I think the most similar documentation we have is this section: https://docs.djangoproject.com/en/5.0/ref/contrib/admin/#theming-support

Perhaps we could add another section underneath it?

@stefanivic stefanivic changed the title Ticket 35505 Refs #35505 -- Added {% extra_body %} template tag. Jun 8, 2024
@stefanivic
Copy link
Contributor Author

I think the most similar documentation we have is this section: https://docs.djangoproject.com/en/5.0/ref/contrib/admin/#theming-support

Perhaps we could add another section underneath it?

Agreed, I have added the documentation in the latest change. Let me know if I need to work a bit on the wording.

@stefanivic stefanivic requested a review from knyghty June 9, 2024 08:29
Copy link
Member

@knyghty knyghty left a comment

Choose a reason for hiding this comment

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

Everything seems okay to me though I'm also not sure on the wording.

@stefanivic
Copy link
Contributor Author

stefanivic commented Jun 9, 2024

Everything seems okay to me though I'm also not sure on the wording.

Thanks for the feedback, I improved the wording a bit to match the style of Django documentation a bit better. Let me know what you think.

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @stefanivic 👍
This looks great, I have a few minor comments

docs/ref/contrib/admin/index.txt Outdated Show resolved Hide resolved
docs/ref/contrib/admin/index.txt Outdated Show resolved Hide resolved
docs/ref/contrib/admin/index.txt Outdated Show resolved Hide resolved
tests/admin_views/tests.py Outdated Show resolved Hide resolved
Comment on lines 124 to 125

{% block extra_body %}{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% block extra_body %}{% endblock %}
{% block extrabody %}{% endblock %}

Considering we have extrastyle and extrahead, this should perhaps be called extrabody

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahboyce i was going after extra_js but considering the extrahead block, I agree. It's a better match.

@stefanivic
Copy link
Contributor Author

stefanivic commented Jun 13, 2024

@sarahboyce thank you for taking the time to write a great review. I agree with the proposed changes and have implemented them. Let me know what you think.

Also let me know if this needs a squash.

@sarahboyce sarahboyce changed the title Refs #35505 -- Added {% extra_body %} template tag. Fixed #35505 -- Added extrabody block to admin/base.html. Jun 17, 2024
@sarahboyce sarahboyce added the selenium Apply to have Selenium tests run on a PR label Jun 17, 2024
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @stefanivic 🥳 this looks great
I pushed minor tweaks 👍

@sarahboyce sarahboyce merged commit ce1ad98 into django:main Jun 18, 2024
28 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DjangoCon Europe 🏰 selenium Apply to have Selenium tests run on a PR
Projects
None yet
4 participants