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 #35115 -- Made admin's footer render in <footer> tag. #17774

Merged
merged 5 commits into from Jan 24, 2024

Conversation

MHLut
Copy link
Contributor

@MHLut MHLut commented Jan 23, 2024

Fixes ticket #35115.

Important notes:

  • Removes all existing footer CSS per recommendations in the ticket.
  • Uses the semantic element plus ID of the same name in the same style as the page header.
  • Puts the opening tags inside the block tags, also in the same style as the page header.
  • The now-removed CSS clear on the footer did nothing since the parent element is a flexbox.

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 ⛵️!

</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!

@felixxm felixxm changed the title Fixed #35115 -- Added semantic footer outside of main in admin Fixed #35115 -- Made admin's footer render in <footer> tag. Jan 24, 2024
</main>
</div>
<footer id="footer">{% block footer %}{% endblock %}</footer>
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a small test? For example:

diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py
index cb61c88941..429a79c062 100644
--- a/tests/admin_views/tests.py
+++ b/tests/admin_views/tests.py
@@ -1605,6 +1605,10 @@ class AdminViewBasicTest(AdminViewBasicTestCase):
             '<main id="content-start" class="content" tabindex="-1">',
         )
 
+    def test_footer(self):
+        response = self.client.get(reverse("admin:index"))
+        self.assertContains(response, '<footer id="footer">')
+
     def test_aria_describedby_for_add_and_change_links(self):
         response = self.client.get(reverse("admin:index"))
         tests = [

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added a test based on the existing header test!

docs/releases/5.1.txt Outdated Show resolved Hide resolved
MHLut and others added 2 commits January 24, 2024 10:13
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm felixxm added the selenium Apply to have Selenium tests run on a PR label Jan 24, 2024
@felixxm
Copy link
Member

felixxm commented Jan 24, 2024

@MHLut Thanks 👍 Welcome aboard ⛵

@felixxm felixxm merged commit e412d85 into django:main Jan 24, 2024
39 checks passed
@MHLut
Copy link
Contributor Author

MHLut commented Jan 24, 2024

@felixxm Thank you for the help! Much appreciated.

@MHLut MHLut deleted the ticket_35115 branch January 24, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Djangonauts 🚀 selenium Apply to have Selenium tests run on a PR
Projects
None yet
3 participants