Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#34995 closed Bug (fixed)

Add link for admin's related widget is in the wrong place

Reported by: Tom Carrick Owned by: Tom Carrick
Component: contrib.admin Version: 5.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description (last modified by Tom Carrick)

https://code.djangoproject.com/attachment/ticket/34995/Screenshot%202023-11-23%20at%2016.01.04.png

Attachments (1)

Screenshot 2023-11-23 at 16.01.04.png (75.8 KB ) - added by Tom Carrick 6 months ago.

Download all attachments as: .zip

Change History (16)

by Tom Carrick, 6 months ago

comment:1 by Tom Carrick, 6 months ago

Description: modified (diff)
Owner: changed from nobody to Tom Carrick
Status: newassigned

comment:2 by Tom Carrick, 6 months ago

Has patch: set
Last edited 6 months ago by Tom Carrick (previous) (diff)

comment:3 by Tom Carrick, 6 months ago

Version: 4.25.0

Regression in 1699f8b52ac15f687cc39088401c2641022c71cd.

So I think this is a release blocker for 5.0, not 4.2?

comment:4 by Natalia Bidart, 6 months ago

Triage Stage: UnreviewedAccepted
UI/UX: set

Agreed, this is a 5.0 release blocker.

comment:5 by Natalia Bidart, 6 months ago

Triage Stage: AcceptedReady for checkin

comment:6 by Natalia Bidart, 6 months ago

Triage Stage: Ready for checkinAccepted

Getting a selenium failure that needs debugging.

comment:7 by Mariusz Felisiak, 6 months ago

Severity: Release blockerNormal

IMO we shouldn't treat this as a release blocker. "+" always had various issues, you can play with font and screen sizes and you'll notice various incorrect behaviors in the older versions of Django. I don't believe that Django 5.0 made it significantly worse. Also, there is a huge risk that the proposed patch will introduce further regressions. Let's don't backport it.

in reply to:  7 comment:8 by Mariusz Felisiak, 6 months ago

Replying to Mariusz Felisiak:

IMO we shouldn't treat this as a release blocker. "+" always had various issues, you can play with font and screen sizes and you'll notice various incorrect behaviors in the older versions of Django. I don't believe that Django 5.0 made it significantly worse. Also, there is a huge risk that the proposed patch will introduce further regressions. Let's don't backport it.

I leave the final decision to Natalia, I'm not going to die on this hill.

comment:9 by Tom Carrick, 6 months ago

Personally, I don't think this needs to be in 4.2.x. However, I would like to see it in 5.0. I promise to fix any regressions!

comment:10 by Natalia <124304+nessita@…>, 6 months ago

In af2fd36:

Refs #34995 -- Made Selenium tests more robust for admin_views and admin_widgets suites.

Depending on screen sizes, the selenium tests that would "click" or interact
with an element outside the scope of the visible window would produce test
failures (raising ElementNotInteractableException in CI runs).

This branch switches those to using ActionChains, which ensures that the click
(or other interaction) is performed only after successfully completing the
move to the relevant element.

Co-authored-by: Tom Carrick <tom@…>

comment:11 by Natalia <124304+nessita@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 999ba9d:

Fixed #34995 -- Improved position of related widget's add link on admin pages on small screens.

Regression in 1699f8b52ac15f687cc39088401c2641022c71cd.

Co-authored-by: Sarah Boyce <42296566+sarahboyce@…>
Co-authored-by: Natalia Bidart <124304+nessita@…>

comment:12 by Natalia Bidart, 6 months ago

Triage Stage: AcceptedReady for checkin

I have merged the PR #17517 in main and will review how a potential backport would look like in stable/5.0x. Then I'll report back on next steps. Thanks everyone!

Last edited 6 months ago by Natalia Bidart (previous) (diff)

comment:13 by Natalia Bidart, 6 months ago

I have locally backported and tested 999ba9db6d6331eaa58af77debba42754bcc1a8e in stable/5.0.x and I haven't find any (new) Admin UI issues, only improvements. I have tested small (phone), medium (tablet) and wide (desktop) screens, both horizontal and vertical rotated viewports, light and dark modes, and LTR and RTL languages.
I'm a bit dizzy at this point but I'm fairly confident that with this change, the "green plus sign" is consistently shown to the right of the M2M selector (in LTR langs, it's shown to the left in RTL langs) and I think this is an improvement over what's currently available.

Given the above, and given Tom's explicit commitment to fix any potential regression, I think this is a good change to be included in 5.0. I'll be backporting the mentioned revision and the previous one that make some Selenium tests more robust.

comment:14 by Natalia <124304+nessita@…>, 6 months ago

In 9fe12b0:

[5.0.x] Refs #34995 -- Made Selenium tests more robust for admin_views and admin_widgets suites.

Depending on screen sizes, the selenium tests that would "click" or interact
with an element outside the scope of the visible window would produce test
failures (raising ElementNotInteractableException in CI runs).

This branch switches those to using ActionChains, which ensures that the click
(or other interaction) is performed only after successfully completing the
move to the relevant element.

Co-authored-by: Tom Carrick <tom@…>

Backport of af2fd368156439b79e4c1eb2278c433246771e44 from main

comment:15 by Natalia <124304+nessita@…>, 6 months ago

In 471fa926:

[5.0.x] Fixed #34995 -- Improved position of related widget's add link on admin pages on small screens.

Regression in 1699f8b52ac15f687cc39088401c2641022c71cd.

Co-authored-by: Sarah Boyce <42296566+sarahboyce@…>
Co-authored-by: Natalia Bidart <124304+nessita@…>

Backport of 999ba9db6d6331eaa58af77debba42754bcc1a8e from main

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