Opened 4 years ago

Closed 3 years ago

#18511 closed Cleanup/optimization (fixed)

Admin Site: Error Message displays different for Changeforms and the Password Change screen.

Reported by: Serge Spaolonzi Owned by: Serge Spaolonzi
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

In the Admin site the error message ('Please correct the errors below.') shows in different positions for the changeform screens and the password change screen.
In the Change Form screens it shows bellow the h1 title and in the password change screens it shows above the h1 title.

Change History (11)

comment:1 Changed 4 years ago by Serge Spaolonzi

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Serge Spaolonzi
Patch needs improvement: unset
Status: newassigned

comment:3 Changed 4 years ago by Serge Spaolonzi

Summary: Admin Site: Error Message displays differently for the Changeforms and the Password Change screen.Admin Site: Error Message displays different for Changeforms and the Password Change screen.

comment:4 Changed 4 years ago by Julien Phalip

Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Thanks for the report and patch. It would be more consistent with the rest of the codebase if the title was passed as a context variable from the view to the template. Could you make that change and also submit a pull request? Thanks!

comment:5 in reply to:  4 Changed 4 years ago by Serge Spaolonzi

Version: 1.4master

Replying to julien:

Thanks for the report and patch. It would be more consistent with the rest of the codebase if the title was passed as a context variable from the view to the template. Could you make that change and also submit a pull request? Thanks!

Done.
I have made the changes in a new branch, the branch I posted before was based in 1.4, the new branch is base in master.
https://github.com/cobalys/django/tree/ticket_18511

comment:6 Changed 4 years ago by Julien Phalip

Thank you, the patch looks good. However, I'm wondering, why have two different variables (content_title and title)? Can't we just have one that is used both in <title> and <h1>?

comment:7 in reply to:  6 Changed 4 years ago by Serge Spaolonzi

Replying to julien:

Thank you, the patch looks good. However, I'm wondering, why have two different variables (content_title and title)? Can't we just have one that is used both in <title> and <h1>?

I had to use two variables because the value of <title> and <h1> are not always the same. In the template:
'django/contrib/admin/templates/registration/password_reset_confirm.html' title is always 'Password reset' but the value of 'h1' may be 'Enter new password' or 'Password reset unsuccessful' according to the situation. It was implemented that way and I tried to keep the same values.

comment:8 Changed 4 years ago by Julien Phalip

Ok, I see, thanks for clarifying. I think this is a good opportunity to simplify things a bit by only having one title used both in the <h1> and the <title>. Could you merge those two variables into a single one and keep the title value that makes most sense in each case?

comment:9 in reply to:  8 Changed 4 years ago by Serge Spaolonzi

Replying to julien:

Ok, I see, thanks for clarifying. I think this is a good opportunity to simplify things a bit by only having one title used both in the <h1> and the <title>. Could you merge those two variables into a single one and keep the title value that makes most sense in each case?

The code is updated with those changes, I left only one variable named 'title'.

comment:10 Changed 4 years ago by Serge Spaolonzi

I have modified the code, I have deleted an extra test that was unnecessary.
https://github.com/cobalys/django/tree/ticket_18511

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e07e4030b9170d522a9670a80c4fd40acff369cb:

Fixed #18511 -- Cleaned up admin password reset template titles.

Note: See TracTickets for help on using tickets.
Back to Top