Refs #34038 — Improve link readability and identifiability #17419

yushanfans2233 · 2023-10-28T05:42:55Z

Light

Before

After

Dark

Before

After

github-actions

Hello! Thank you for your contribution 💪

As it’s your first contribution be sure to check out the patch review checklist.

If you’re fixing a ticket from Trac make sure to set the “Has patch” flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

github-actions · 2023-10-28T08:02:57Z

🖼️ Screenshots created

You can download the generated screenshots from the workflow artifacts.

Please note that artifacts are only available for download for 90 days.

Generated screenshots for 2d48d9f at Sat Oct 28 08:02:52 UTC 2023 (download).
Generated screenshots for d49cf8b at Sat Oct 28 09:59:55 UTC 2023 (download).
Generated screenshots for cfc9a2a at Sun Nov 5 12:49:23 UTC 2023 (download).
Generated screenshots for e39473f at Tue Nov 7 14:45:20 UTC 2023 (download).

sarahboyce · 2023-10-28T08:15:56Z

Thank you for this! If you like, you can add a screenshot within the test_user_change_password test (something like self.take_screenshot("user_change")) which will also add this page with light dark mode etc in our tests 👍

yushanfans2233 · 2023-10-28T10:00:38Z

@sarahboyce done, PTAL 😄

sarahboyce · 2023-10-28T11:08:31Z

Thank you @yushanfans2233! This looks right but for some reason I can’t see another screenshot generated 🤔 can you run the new selenium test with screenshots locally (https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/unit-tests/#running-the-selenium-tests) and see if it created screenshots?

yushanfans2233 · 2023-10-28T13:15:07Z

I ran the command .\runtests.py --parallel=1 --selenium=chrome --screenshots.
Then I see the following screenshots created:

test_user_change_password_dark-user_change.png
test_user_change_password_desktop_size-user_change.png
test_user_change_password_mobile_size-user_change.png
test_user_change_password_rtl-user_change.png

sarahboyce

Needs a review from the accessibility team but this looks good to me ⭐

yushanfans2233 · 2023-10-28T14:53:21Z

Thanks for your help, really appreciate it 😸 .

felixxm · 2023-10-30T07:39:03Z

tests/admin_views/tests.py

+        with self.wait_page_loaded():
+            self.selenium.get(self.live_server_url + change_url)
+
+        self.take_screenshot("user_change")


This test doesn’t have any assertions. Instead I’d add take_screenshot to one of existing tests, e.g. UserAdminTest.test_save_button().

UserAdminTest is not a AdminSeleniumTestCase, I can’t take screenshot in it.

sabderemane

Looks good! Thank you for your contribution @yushanfans2233 😄
I just add a comment for the accessibility team

sabderemane · 2023-10-30T07:47:22Z

django/contrib/admin/static/admin/css/base.css

-    color: var(—body-fg);
-    text-decoration: none;
+    color: var(—link-fg);
+    text-decoration: underline;


@knyghty @thibaudcolas I have one concern about this change, I haven’t tested this change yet and can’t see the screenshot on my phone, but it got me thinking about titles that can also be links, are they also considered underlined or just the content?

As far as I’m aware, all links in the admin will be underlined, so this is a big UI change.

Argh, sorry I should have rephrase, I wanted to say : “do we want them also underlined or just the content?”
I don’t know if it’s already the case according the style, but yes all links in the admin will be underlined as you said, but I think we should agree on what we want before to merge this PR

knyghty

Thanks for the work here, it’s definitely needed. Also sorry for the delay.

Clearly the accessibility of the links in the body is improved. I think we do need some overrides though. The main one is that buttons that act as links are currently underlined, e.g. the “delete” button and the “Add Foo” button on the list page as well as the view on site and history buttons.

There are some where I’m less certain. For example the title text (default “Django administration” probably doesn’t need to be underlined. I’d also argue that the links in the navigation sidebar don’t really need to be underlined, either. But again, interested in a second opinion here.

I also see a link that should be underlined, but isn’t:

I think unrelated to this PR, but I also notice the log out link in the header has a fainter underline than the rest of the links, because it’s using border-bottom rather than text-decoration:

yushanfans2233 · 2023-11-03T14:30:39Z

Here are two alternative solutions:

Solution 1

Apply an underline and blue color to all <a> tags, and use a CSS class to remove this style from specific locations where it is not needed.

Pros: Ensures that no links requiring improvement are missed, even if new ones are added in the future.
Cons: May require modifying a significant number of places.

Solution 2

Introduce a new CSS class called .a11y-link and apply it to the links that require improved readability and identifiability.

Pros: Provides more precise targeting without affecting existing elements.
Cons: There is a potential risk of missing some existing links, as well as future additions, when attempting to identify and modify them all.

knyghty · 2023-11-03T20:46:11Z

@yushanfans2233 personally I’d give option 1 a go. It’s better to accidentally add an underline to something than it is to accidentally forget one, and if there are any they can be fixed over time as they’re noticed.

…cted

Main points: 1. The <a> element should be colored and underlined to indicate its ability to navigate to a new page or an anchor within the current page. 2.The <a> element, when used as a button or resembling a button, should not be colored and underlined.

yushanfans2233 · 2023-11-05T12:52:38Z

Hello everyone, I have made some necessary fixes based on the following principles:

To indicate its functionality in navigating to a new page or an anchor within the current page, the <a> element should be styled with color and underlining.
When the <a> element is utilized as a button or appears similar to a button, it should not be styled with color and underlining.

knyghty

One thing I’m not sure about. Maybe it’s fine as it is or maybe not.

At the moment the filters don’t have underline, and the only blue ones are the active filters. Clearly we need to show which filter is active. I’m just not sure which is best. Maybe it’s fine how it is, but it is a bit hard to discern the text there are links.

The Today and Now controls also need something but not sure what. At the moment it’s very unclear you can interact with them. But they don’t really act like links to other pages, they are more like buttons. Again, not sure the best way forward here:

yushanfans2233 · 2023-11-06T13:28:01Z

One thing I’m not sure about. Maybe it’s fine as it is or maybe not.

At the moment the filters don’t have underline, and the only blue ones are the active filters. Clearly we need to show which filter is active. I’m just not sure which is best. Maybe it’s fine how it is, but it is a bit hard to discern the text there are links.
The Today and Now controls also need _something_ but not sure what. At the moment it’s very unclear you can interact with them. But they don’t really act like links to other pages, they are more like buttons. Again, not sure the best way forward here:

Yes, I completely agree with your viewpoint. It is important to enhance the visibility of these elements and make it clear to users that they are clickable functional elements. However, I believe we should create a new ticket to address this improvement as it is not directly related to the changes made in this PR.

knyghty · 2023-11-06T13:40:57Z

For the sidebar filter, I can maybe agree, however there is a regression for the datetime inputs as this PR removes the colour. See this example from 4.2:

yushanfans2233 · 2023-11-07T13:11:30Z

Yes, there is a regression on this link, but I don’t believe it was introduced by my PR. I am running an admin site with the latest code from the Django main branch, and it does not display as blue.

If you want, I can fix it in this PR.

keramblock · 2024-01-08T15:11:24Z

Any news on this PR? We also need that =(

nessita · 2024-01-08T19:10:24Z

Any news on this PR? We also need that =(

As per the latest activity, it seems that this PR needs a review from the @django/accessibility team.

knyghty

We had a good chat about this at our latest @django/accessibility meeting.

We agreed that for now, we should just fix the colours and not add the underline.

We should still do the underline part, but as a new ticket, because:

The changes can easily be done separately.
Because of the difficulty in deciding what should and shouldn’t be underlined, it’s better to get the less contentious change out first.

nessita · 2024-01-17T16:17:58Z

We had a good chat about this at our latest @django/accessibility meeting.

We agreed that for now, we should just fix the colours and not add the underline.

We should still do the underline part, but as a new ticket, because:
* The changes can easily be done separately.

* Because of the difficulty in deciding what should and shouldn't be underlined, it's better to get the less contentious change out first.

Thank you @knyghty. I agree that separating the link color change from the underlining work is a good path forward. Therefore I recommend:

File a new ticket for the color link regression, so I can triage it as a release blocker and we can provide the minimal fix suitable for backporting, and
Potentially re-title the original ticket #34038 to focus on the underlines.

What do you think?

knyghty · 2024-01-17T17:30:43Z

@nessita sounds good on my end

yushanfans2233 · 2024-01-20T03:07:10Z

Dear Django community,

I regret to inform you that due to my current busy schedule, I am unable to continue working on this PR for the time being. However, please know that I remain eager to assist if needed on any other project within the community.
So I suggest to close this PR and transfer the ticket to others.

Thank you for your understanding.

nessita · 2024-01-20T19:17:01Z

Dear Django community,

I regret to inform you that due to my current busy schedule, I am unable to continue working on this PR for the time being. However, please know that I remain eager to assist if needed on any other project within the community. So I suggest to close this PR and transfer the ticket to others.

Thank you for your understanding.

No worries @yushanfans2233, we appreciate everything you have done so far! Thank you for letting us know.

fix: add an underline and switch to a better contrast

2d48d9f

felixxm requested a review from a team October 28, 2023 05:44

felixxm changed the title ~~Fixed #34038 — Improve link readability and identifiability~~ Refs #34038 — Improve link readability and identifiability Oct 28, 2023

github-actions bot reviewed Oct 28, 2023

View reviewed changes

sarahboyce added the screenshots 🖼️ label Oct 28, 2023

test: add screenshot test

d49cf8b

sarahboyce approved these changes Oct 28, 2023

View reviewed changes

felixxm reviewed Oct 30, 2023

View reviewed changes

sabderemane reviewed Oct 30, 2023

View reviewed changes

knyghty self-requested a review November 1, 2023 19:08

knyghty reviewed Nov 2, 2023

View reviewed changes

yushanfans2233 added 2 commits November 5, 2023 20:32

fix: <a> in calendar picker and datetime shortcuts should not be affe…

df0642b

…cted

fix: styling for <a> element

cfc9a2a

Main points: 1. The <a> element should be colored and underlined to indicate its ability to navigate to a new page or an anchor within the current page. 2.The <a> element, when used as a button or resembling a button, should not be colored and underlined.

knyghty self-requested a review November 5, 2023 12:56

knyghty reviewed Nov 5, 2023

View reviewed changes

yushanfans2233 added 2 commits November 7, 2023 22:41

fix: add-related-record link

666b9a6

fix: paginator button and show-all link

e39473f

knyghty self-requested a review November 7, 2023 14:51

felixxm requested a review from a team December 27, 2023 12:29

knyghty suggested changes Jan 13, 2024

View reviewed changes

nessita closed this Jan 20, 2024

티스토리 수익 글 보기

티스토리 수익 글 보기

Uh oh!

Refs #34038 — Improve link readability and identifiability #17419

Refs #34038 — Improve link readability and identifiability #17419

Uh oh!

Conversation

yushanfans2233 commented Oct 28, 2023

Light

Before

After

Dark

Before

After

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarahboyce commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yushanfans2233 commented Oct 28, 2023

Uh oh!

sarahboyce commented Oct 28, 2023

Uh oh!

yushanfans2233 commented Oct 28, 2023

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

yushanfans2233 commented Oct 28, 2023

Uh oh!

felixxm Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

yushanfans2233 Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

sabderemane left a comment

Choose a reason for hiding this comment

Uh oh!

sabderemane Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

felixxm Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

sabderemane Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knyghty left a comment

Choose a reason for hiding this comment

Uh oh!

yushanfans2233 commented Nov 3, 2023

Solution 1

Solution 2

Uh oh!

knyghty commented Nov 3, 2023

Uh oh!

yushanfans2233 commented Nov 5, 2023

Uh oh!

knyghty left a comment

Choose a reason for hiding this comment

Uh oh!

yushanfans2233 commented Nov 6, 2023

Uh oh!

knyghty commented Nov 6, 2023

Uh oh!

yushanfans2233 commented Nov 7, 2023

Uh oh!

keramblock commented Jan 8, 2024

Uh oh!

nessita commented Jan 8, 2024

Uh oh!

knyghty left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 28, 2023 •

edited

Loading

sarahboyce commented Oct 28, 2023 •

edited

Loading

sabderemane Oct 30, 2023 •

edited

Loading