I've updated the failing tests. @felixxm is the approach of the fix okay?

I could be wrong but I thought @thibaudcolas' recommendation was to look at just making the small-width responsive CSS be used for all widths. I didn't think any HTML changes were required with this? 🤔

You're right, my apologies! I read that comment, but it slipped out of my mind when I was working on the patch 😅

Thanks for the review, @knyghty 👍🏻
I've removed the extra padding given by the .wide class and the extra margin for the .wide class in RTL.

I also found these lines in forms.css that add the same margin to paragraphs and divs with the .help class, but I couldn't find a page or form state on the admin site where these styles were applied. I've preemptively removed those lines as well.

However, now that form labels are placed above their respective inputs, validation errors on fields are displayed above the labels, like this -

IMO, we should move these errors to be below the inputs and above the help text (if any).

@hrushikeshrv thanks for bringing this up, it's indeed in a bad place. I talked with @thibaudcolas a bit and we suggest following gov.uk and putting it between the help text and the input: https://design-system.service.gov.uk/components/error-message/

It might also be a good time to move the help text to the same place (above the input, rather than below).

I also wonder if the aligned and wide classes are meaningless now. wide in particular is an issue because it's documented: https://docs.djangoproject.com/en/4.2/ref/contrib/admin/#django.contrib.admin.ModelAdmin.fieldsets

I also wonder if the aligned and wide classes are meaningless now.

I agree. .wide mainly just increases the with of labels from 160px to 200px and decreases the width of .vLargeTextField and .vXMLLargeTextField - this generally won't make a visual difference unless the label is very long.

Without the wide class -

With the wide class -

IMO we should increase the width of the labels to 200px anyway, now that they are placed above the fields instead of beside them.

There are only 3 selectors for .wide in forms.css which largely have no noticeable effect now (from the cases I have seen).

I'm not so sure about .aligned being meaningless now, since it defines very specific margin and padding numbers for most form elements.

@hrushikeshrv my feeling is we should just remove the label widths. They might be excessively long in some cases I suppose, but I feel like intuitively they should take up the full width of the content. If it feels too long for very verbose labels, maybe that's a great impetus to increase the font size (in another PR).

For .aligned, maybe, I didn't look into it too much. Removing widths is probably enough for here and we can do any clean up, again, in another PR.

@knyghty I tested defining multiple fields on the same line, and it was indeed not working. I've handled that case so it now works as expected.

Regarding the wide class, it used to work by setting a width of 200px for field labels, which pushed the field inputs to the right (or left in case of rtl), leading to more horizontal space. Now that the labels are on top of the inputs, just setting the label widths won't lead to more horizontal space, so I've modified the effect of the wide class to instead set an increased margin on the fieldBox class, which leads to more horizontal space. I think this is the right approach, and I've checked to make sure that there aren't any overflow issues on different screen sizes, but please do confirm.

I will do some testing myself but I think otherwise this is ready for a review from someone other than me. I've updated the flags on the ticket. Release notes are probably also required.

Thanks @knyghty. I've fixed the merge conflicts as well.

This is my first time writing release notes so I had a few questions. Should I write the release notes in docs/releases/5.1.txt? If so, would writing them under the django.contrib.admin heading be okay? 😅

Thanks @knyghty. I've fixed the merge conflicts as well.

This is my first time writing release notes so I had a few questions. Should I write the release notes in docs/releases/5.1.txt? If so, would writing them under the django.contrib.admin heading be okay? 😅

Yes, release notes should be in the 5.1.txt file following the existing style (verb tense, etc). The django.contrib.admin is correct as well.

Also, could you please rebase your PR onto main? This mean first of all, updating your main branch with the latest revisions and then running a command like this: git rebase main in your branch. More details in this blogpost.

When all of the above is complete, please make sure that the title references the corresponding ticket and to adjust the ticket flags in the Trac system so this PR gets added to the "branches needing review" section of the Django Developer Dahsboard. More information in these docs.

If you need any help, please ask! Thank you, Natalia.

After an initial review, I see these issues in the admin UI. Let me know your thoughts!

• Different widths for groups and permissions for User:
  

• Help text on top of (read only) field is very confusing about which phrase is help and which phrase is the field value:
  

• Since we are gaining horizontal space for the fields, it seems like it would be a good idea to let them grow horizontally as wide as they can, so we avoid long texts to the chopped off like this:
  

Hello @hrushikeshrv, would you have time to keep working on this? Thank you for your efforts so far!

Hi @nessita, sorry for the late response! I was a little swamped for the last few weeks, but I will be able to work on this patch now. I'll rebase this PR and address the width and horizontal space concerns asap!

Hi @nessita, sorry for the late response! I was a little swamped for the last few weeks, but I will be able to work on this patch now. I'll rebase this PR and address the width and horizontal space concerns asap!

Thank you! I did not mean to rush you, just knowing that you'll work on this when you can is enough. Good luck!

Since we are gaining horizontal space for the fields, it seems like it would be a good idea to let them grow horizontally as wide as they can, so we avoid long texts to the chopped off

I agree, I've implemented this and it fixes the issue of different widths for groups and permissions as well. The permissions widget was appearing wider because it was growing to try to fit its content.

However, now that both the widgets grow to fill the entire width, the link to add a related object (the ➕ button) seems a little out of place IMO -

Moreover, on mobile layouts, it appears even more disconnected after this patch -

This may be out of scope for this ticket, but what if we changed that link to appear alongside the Available groups heading, like so -

Please let me know if that seems reasonable or if I should try another approach.

Hey @hrushikeshrv, thank you for restarting work on this.

Specifically about the "Add" (plus sign) button: I love the new proposed location. Do you think that moving it to that place could be done in isolation/independently of this branch? I'm wondering we could do a separated commit/PRs with that.

Of course we should also confirm with the accessibility team (cc @django/accessibility @knyghty) that this "Add" button change is acceptable. Did you happen to check screen readers and tab keyboard navigation?

@nessita Yes, I think we could do this independently in a separate PR. I haven't checked with screen readers or tab keyboard navigation yet. I can do that and create a ticket if the accessibility team agrees.

@hrushikeshrv @nessita, I don't see an immediate problem with the new position - I do think it is actually easier to see from a UX perspective and good to have a clear text label as well. However, it's easier to review the accessibility in PR form, so I'd certainly encourage creating one (with a ticket).

Hi @nessita, I tried to rebase this branch onto main, but it looks like I messed up somehow. I followed the steps you mentioned and the steps in Adam's article, but it looks like I committed every change that has been made since I created my branch.
Sorry for my mixup, I'm not sure how to fix this 😅

Hi @nessita, I tried to rebase this branch onto main, but it looks like I messed up somehow. I followed the steps you mentioned and the steps in Adam's article, but it looks like I committed every change that has been made since I created my branch. Sorry for my mixup, I'm not sure how to fix this 😅

No worries, let's see: what command did you run, and how did you resolve conflicts, if there were any?

In general, you can undo changes to your git repo by (make a copy of your work before proceeding, just in case). See more, for example, in this stackoverflow link:

• Run git reflog and read carefully the list of changes shown there. There will be a line for the latest known "good state" of your branch, something like:

<a hash> (origin/ticket_34643) HEAD{@<number>}: <action description>

• Write down that hash (for example a123b456c7, likely belonging to the last good commit you made.
• Reset your git tree to that revno. Careful that this reset is a "hard" reset, changes newer than a123b456c7 will be lost:

git reset --hard a123b456c7

With that, your git tree will look like it was at a123b456c7. Then, you can try the rebase again (git rebase main).

@knyghty Sorry for the inactivity. I will be able to work on this, but I might be delayed, since I have just moved, and I'm still in the process of setting up my new house.

Hi @sarahboyce, I've added some extra margin between labels and inputs and also updated the release notes. If this looks good, I can generate the screenshots. Let me know!

Thank you for the updates!

To me it looks like the margin between the fields and the label still needs some work. Looking at the screenshots generated from the visual regression tests you can see this margin appears to be missing in some cases

The "Start date" and "Start time" fields looks great but "Main band" and "Description" look to be quite close to their input - I think the spacing should be consistent and I would like to see similar spacing as to the "Start date" and "Start time" fields

Can you also add a screenshot test cases for errors? This will help ease the review (see the screenshots section of running the selenium tests docs for some context)

Thanks for the review, I'll see what's going wrong. I'll add screenshot test cases for errors as well

Hi @sarahboyce, is it okay to add the new tests in tests/admin_views/tests.py in the SeleniumTests class?

Hi @sarahboyce, is it okay to add the new tests in tests/admin_views/tests.py in the SeleniumTests class?

Yes 👍

I've added the screenshot test to check for error message positions. I've checked the screenshots locally as well and they seem to be coming out right. Can you confirm @sarahboyce?

Thank you @hrushikeshrv ⭐
This looks really strong and getting the screenshots both locally and in the CI
I have squashed the commits and done a minor tweak to a test

I have two questions

I see that our multi widgets (see "Date:" and "Time:" below) will have the labels next to the field rather than above
How do we feel about this? Should those also change? Is there a reason why this shouldn't change? (cc @django/accessibility)

A non-blocking thought is "why do the labels end with a colon : ?" If the reason was due to the layout where the label was on the left, then we perhaps could/should remove it.

@thibaudcolas is at the sprint I have have asked if he could also do some testing but this is very positive and I think we could generate the screenshots for the docs

I'll generate the screenshots for the docs 👍🏻

Comparing side by side, IMO having the labels for date and time inputs next to the fields feels better and more "continuous". Also, the multi widgets will have a label above them for the actual field name -

About the date and time fields, those two may need to be wrapped in a <fieldset> with the overlapping label as a <legend>. (Looking at the other accessibility members for a second opinion.)

Such a change is out of scope for this PR, though.

FWIW I would also prefer the labels to not have a colon but also keeping the date and time fields roughly the way they are, which might be better with a colon. Which is probably the most annoying thing I could ask for :)

But I don't think this should be blocking, I'd be happy with something like this in a follow-up

@sarahboyce For the docs screenshots, should I use the same models as the current screenshots? If so, where can I find those models?

I'll generate the screenshots for the docs 👍🏻

Comparing side by side, IMO having the labels for date and time inputs next to the fields feels better and more "continuous". Also, the multi widgets will have a label above them for the actual field name -

We also have a AdminDateWidget and a AdminTimeWidget, I think for consistency the help text for your time should be under the label/legend but we can keep that the labels are to the side when it is a fieldset

I also want an approval from the accessibility team so re-requested a review here 👍

Summary of work needed to land this:

• address the help text of Date and Time admin widgets (see comment)
• approval from accessibility team member
• need to confirm where to put the release notes (see comment) - @nessita do you have a preference here?

The updates to screenshots is a nice to have in this PR but not blocking and can be addressed later

Happy to have another look, but will wait until the final widgets are done.

Hi @sarahboyce, I ran into a few issues with merge conflicts and rebasing, and I mistakenly erased some history by force pushing. I've tried for a while to try to fix things, but I'm not able to. Is it okay if I close this PR and open a new one from a new branch from current main? I have a backup of all changes made in this PR until now.

Sorry for the inconvenience 😅

Yeah that's fine - do what you need to do 👍

Hi @hrushikeshrv, curious if you are still working on this? I would love to continue if not?
CC @sarahboyce

Hi @YounesDjelloul, I'm unfortunately not able to work on this PR at the moment, please do feel free to take over 👍🏻

Hi, new here and exploring the codebase, would love to get a little help! I have forked the main branch and tried to run the selenium tests locally! Though some of them are failing, is this normal or am I missing something?

Hello @YounesDjelloul , if you’re not planning to work on this, would it be okay if I give it a try?