django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #35489 -- Fixed vertical alignment of raw_id_fields widget.

Open Atem18 opened this issue 2 years ago • 14 comments

In the modeladmin, when using raw_id_fields = ("parent",) in the ModelAdmin, the search button and name are aligned at the top.

This PR fixes that allign.

Before image

After image

Atem18 avatar Dec 29 '23 11:12 Atem18

Hello @Atem18, thank you for your interest in making Django better.

While I agree that the search button and corresponding text look better when they are center-aligned, the change you are proposing is affecting many other admin pages and pieces. For example, see the User detail page (see how the field labels change from top aligned to centered):

  • In main: image

  • In this branch: image

nessita avatar Jan 03 '24 14:01 nessita

@nessita Hi, thanks for reviewing my PR. For me it looks great to have them centered instead of top-aligned because when you look at other smaller fields, they looks like aligned in the center and not the top: image

I guess this should be fixed anyway because in forms.css, the .related-lookup class has the property vertical-align: middle; but it does not work and Firefox recommends to fix it: image image

Atem18 avatar Jan 05 '24 09:01 Atem18

Hello @Atem18!

I consulted with a Django contributor who has worked recently on the admin CSS and that is also part of the accessibility team, and they laid our a few points I agree with:

  • Labels aligned at the top are better for scanning a page top-bottom. If there is a big fieldset like the one for permissions, on smaller screens a user may see the field but not the label and that could be confusing.
  • The alignment of text/search icon with the label is nice but we don't think it's worth it for the regression on larger fields.

Perhaps it would be better to align the input text better in a top aligned mode though it may not be worth the effort. Could you think of another way of aligning that icon?

nessita avatar Jan 06 '24 12:01 nessita

Hi @nessita, thanks a lot for the comments, I agree with it indeed. As for the lookup, they were properly aligned in at least the version 4.1.7 of Django. Before, they were not wrapped in the flex-container and it looked just nearly like my "after" screenshot: image I can do some tests to see in which version this changed but maybe the person who worked on it will know quicker than I do.

Atem18 avatar Jan 06 '24 13:01 Atem18

(this isn’t an accessibility issue so I’ve untagged the team as reviewers. Tom has volunteered himself instead)

thibaudcolas avatar Jan 11 '24 19:01 thibaudcolas

Same thing for the ForeignKey.

In 4.1.7: image

In 5.0.1: image

Atem18 avatar Jan 21 '24 22:01 Atem18

I can confirm that it changed in the version 5.0, it was fine in the 4.2.9

Atem18 avatar Jan 21 '24 22:01 Atem18

diff --git a/django/contrib/admin/static/admin/css/forms.css b/django/contrib/admin/static/admin/css/forms.css
index 539a11ae61..be1a1b8174 100644
--- a/django/contrib/admin/static/admin/css/forms.css
+++ b/django/contrib/admin/static/admin/css/forms.css
@@ -491,7 +491,8 @@ body.popup .submit-row {
 .related-lookup {
     margin-left: 5px;
     display: inline-block;
-    vertical-align: middle;
+    margin-top: auto;
+    margin-bottom: auto;
     background-repeat: no-repeat;
     background-size: 14px;
 }

For me, something like this works. We need to check if other instances with vertical-align: middle; need to be updated 👍

sarahboyce avatar May 31 '24 06:05 sarahboyce

@Atem18 are you planning to submit an updated patch? I didn't want to assume since it's been a few months since this ticket was active.

samGbos avatar May 31 '24 14:05 samGbos

@samGbos I can but I don't know if the solution by @sarahboyce is acceptable. If it is yes, then for sure I update my PR. :)

Atem18 avatar May 31 '24 14:05 Atem18

Hey @Atem18 I have tried out the solution suggested by @sarahboyce. Are you planning to update your PR with that solution?

VaarunSinha avatar Jun 20 '24 13:06 VaarunSinha

Hey @Atem18 I have tried out the solution suggested by @sarahboyce. Are you planning to update your PR with that solution?

Ok, I will do it then, I was waiting on someone giving me the go to do it.

Atem18 avatar Jun 20 '24 14:06 Atem18

Hey @Atem18 have you had the time to update your PR?

VaarunSinha avatar Jun 27 '24 05:06 VaarunSinha

Hi @VaarunSinha, not yet, I hope to do it this week.

Atem18 avatar Jun 27 '24 06:06 Atem18

Hey @Atem18 have you had the time to update your PR yet?

VaarunSinha avatar Jul 02 '24 16:07 VaarunSinha

Hi @VaarunSinha Not yet, I had a busy weekend and will have a busy week + I am sick. Let's see if I can advance on this for this week.

Atem18 avatar Jul 02 '24 18:07 Atem18

I installed the latest Django from git and the bug is gone, it is properly aligned again.

Atem18 avatar Jul 03 '24 21:07 Atem18

Got it thank you for your time and contribution anyway! Get well soon!

VaarunSinha avatar Jul 04 '24 05:07 VaarunSinha

@VaarunSinha @Atem18 I don't think this is fixed in latest Django, this is still an issue in main so if anyone wants to to pick this up, please assign the ticket to yourself. Thank you everyone! image

nessita avatar Jul 05 '24 13:07 nessita

I have a fix ready I will create a PR today.

VaarunSinha avatar Jul 05 '24 15:07 VaarunSinha

Created a PR : https://github.com/django/django/pull/18344

VaarunSinha avatar Jul 05 '24 15:07 VaarunSinha