Skip to content

Fixed #34643 -- Moved admin form label to a more accessible place. #19713

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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fixed #34643 -- Moved admin form label to a more accessible place. #19713

wants to merge 4 commits into from

Conversation

Antoliny0919
Copy link
Contributor

@Antoliny0919 Antoliny0919 commented Aug 8, 2025 •
edited
Loading

Trac ticket number

ticket-34643

Branch description

Continue PR...

Thank you for working on this for such a long time @hrushikeshrv ❤️

I have modified the internal structure of the admin form fields.(more accessible) 🙌🙌🙌

new_field_example

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.

@Antoliny0919 Antoliny0919 force-pushed the moved_admin_form_label branch 2 times, most recently from ebb0cfd to afe5bd6 Compare August 8, 2025 02:35
@Antoliny0919
Copy link
Contributor Author

Antoliny0919 commented Aug 8, 2025 •
edited
Loading

Changed

Before

before_34643(1)

After

after_34643

@Antoliny0919 Antoliny0919 force-pushed the moved_admin_form_label branch 4 times, most recently from 4b7c2b6 to 029beaa Compare August 8, 2025 12:09
@Antoliny0919 Antoliny0919 force-pushed the moved_admin_form_label branch from 029beaa to 69cf786 Compare August 8, 2025 12:35
@sarahboyce sarahboyce added selenium Apply to have Selenium tests run on a PR screenshots 🖼️ labels Aug 8, 2025
sarahboyce
sarahboyce reviewed Aug 8, 2025
View reviewed changes
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.

  • Note that when the PR is close to being merged, we should update the admin doc images
  • Please make @hrushikeshrv a co-author of the commit
  • On the previous PR I mentioned the Date/Time widget having the date and time label next to the input, this is the case in this PR (#16975 (comment)). However, I imagine when #19678 is merged, the layout would change (as currently Date/Time are in <p> tags rather than labels). We need to agree what is wanted and then make sure this stays consistent after changes in other PRs

django/contrib/admin/static/admin/css/forms.css Outdated
@@ -4,7 +4,7 @@

.form-row {
overflow: hidden;
padding: 10px;
padding: 10px 0;
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
padding: 10px 0;
padding: 10px;

I believe we should revert this, otherwise the inline alignment in particular looks very strange
Screenshot from 2025-08-08 15-41-45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s a good point. Thank you.

tests/admin_views/tests.py Outdated
self.live_server_url + reverse("admin:admin_views_language_add")
)
save = self.selenium.find_element(By.CSS_SELECTOR, "div.submit-row input")
save.click()
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
save.click()
with self.wait_page_loaded():
save.click()

Possibly

docs/releases/6.0.txt
@@ -477,6 +477,15 @@ Miscellaneous
* The :option:`collectstatic --clear` command now reports only a summary of
deleted files when ``--verbosity`` is 1. To see the details for each file
deleted, set the ``--verbosity`` flag to 2 or higher.
* In order to improve accessibility of the admin change forms:
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
* In order to improve accessibility of the admin change forms:
* In order to improve accessibility of the admin change forms:

@Antoliny0919
Copy link
Contributor Author

Antoliny0919 commented Aug 8, 2025 •
edited
Loading

  • Note that when the PR is close to being merged, we should update the admin doc images

Yes, I’m aware of this.
However, I’d like to do this at the very end.
If it’s okay, could you give me a heads up right before this work gets approved?

Yes, I’ll add it :)

Yes, is the layout you had in mind when you commented on the previous PR something like this?

before_pr_example

It’s likely that once #19678 is merged, the layout will change to the form shown above.

Personally, I feel that for consistency, the date and time labels should be structured like the above.
Also, while working on this, I felt that I wanted to remove the colon.
Having a colon definitely makes the field label clearer, but if the label is still clearly perceived without the colon, I would prefer to remove it.
For now, I will remove the colon in this change and also adjust the structure of the date and time fields.

For reference, when adjusting the layout of the date and time fields, I will add a label tag and apply some minor styling.
I’m not sure if this change is directly related to this PR, but regardless, I believe it is the right direction from multiple aspects (accessibility, layout).

Antoliny0919 and others added 2 commits August 9, 2025 09:50
@Antoliny0919 @hrushikeshrv
Add sarah suggestion.
665a7a0 
Co-authored-by: Hrushikesh Vaidya <hrushikeshrv@gmail.com>
@Antoliny0919
Fix datetime widget label layout.
182370a
@Antoliny0919
Copy link
Contributor Author

Antoliny0919 commented Aug 9, 2025 •
edited
Loading

Colon (O)

Screenshot 2025-08-09 at 11 19 08 AM Screenshot 2025-08-09 at 11 19 22 AM

Colon (X)

Screenshot 2025-08-09 at 11 19 51 AM Screenshot 2025-08-09 at 11 20 21 AM

Antoliny0919
Antoliny0919 commented Aug 9, 2025
View reviewed changes
django/contrib/admin/templates/admin/widgets/split_datetime.html
@@ -1,4 +1,4 @@
<p class="datetime">
Copy link
Contributor Author

@Antoliny0919 Antoliny0919 Aug 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we change this tag to a div someday?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarahboyce sarahboyce sarahboyce left review comments

Assignees
No one assigned
Labels
screenshots 🖼️ selenium Apply to have Selenium tests run on a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Antoliny0919 @sarahboyce