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

Fixed #35115 -- Made admin's footer render in <footer> tag. #17774

Merged
merged 5 commits into from Jan 24, 2024
Merged

Fixed #35115 -- Made admin's footer render in <footer> tag. #17774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b281efb
Fixed #35115 -- Added semantic footer outside of main in admin
MHLut Jan 23, 2024
5a9c2ca
Remove #footer from responsive.css
MHLut Jan 23, 2024
7a7f9e5
Move the block tags inside the admin footer
MHLut Jan 23, 2024
f4053d3
Update docs/releases/5.1.txt
MHLut Jan 24, 2024
3ab7e1b
Add unit test for admin footer presence
MHLut Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions django/contrib/admin/static/admin/css/base.css
View file
Open in desktop
Expand Up @@ -893,11 +893,6 @@ a.deletelink:focus, a.deletelink:hover {
}
}

#footer {
clear: both;
padding: 10px;
}
MHLut marked this conversation as resolved.
Show resolved Hide resolved

/* COLUMN TYPES */

.colMS {
Expand Down
2 changes: 1 addition & 1 deletion django/contrib/admin/templates/admin/base.html
View file
Open in desktop
Expand Up @@ -108,9 +108,9 @@
<br class="clear">
</div>
<!-- END Content -->
{% block footer %}<div id="footer"></div>{% endblock %}
</main>
</div>
{% block footer %}<footer id="footer"></footer>{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

I thought we agreed on a different approach 🤔

Suggested change
{% block footer %}<footer id="footer"></footer>{% endblock %}
<footer id="footer">{% block footer %}{% endblock %}</footer>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I missed Thibaud's +1 on this decision. I disagree on this method for two reasons:

  1. It makes the footer element itself not overridable.
  2. The other elements, like the page header, also have the element defined within the block, so it feels more consistent to keep it that way.

Should we put this up for discussion here on in the ticket?

Copy link
Member

Choose a reason for hiding this comment

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

It makes the footer element itself not overridable.

So, exactly the same as situation as with <main>.

Copy link
Member

Choose a reason for hiding this comment

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

Per Thibaud's comment it looks like we're doing folks a favor by forcing <footer> here:

Is it clear enough to people who would override this, that with it outside main, they’d definitely need to use a <footer> tag? Otherwise their content would be outside a landmark (​https://dequeuniversity.com/rules/axe/4.3/region).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the block tags inside the element.

Copy link
Member

Choose a reason for hiding this comment

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

We can ask @thibaudcolas for the confirmation, if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK, either solution has pros and cons, I can understand enforcing the footer tags like this!

</div>
<!-- END Container -->

Expand Down
4 changes: 4 additions & 0 deletions docs/releases/5.1.txt
View file
Open in desktop
Expand Up @@ -348,6 +348,10 @@ Miscellaneous
* In order to improve accessibility, the admin's changelist filter is now
rendered in a ``<nav>`` tag instead of a ``<div>``.

* In order to improve accessibility, the admin's footer is now rendered in
a ``<footer>`` tag instead of a ``<div>``. It is now located below the
``<div id="main">`` element.
MHLut marked this conversation as resolved.
Show resolved Hide resolved

* :meth:`.SimpleTestCase.assertURLEqual` and
:meth:`~django.test.SimpleTestCase.assertInHTML` now add ``": "`` to the
``msg_prefix``. This is consistent with the behavior of other assertions.
Expand Down