Fixed #36386 -- Added styles and icons for INFO and DEBUG messages in admin site #19465
Conversation
90917a7 to
e7a9578
Compare
e7a9578 to
05d5cd7
Compare
There was a problem hiding this comment.
Thank you @michalpokusa
I have written a screenshot test we can use to generate screenshots for the UI:
--- a/tests/admin_views/tests.py
+++ b/tests/admin_views/tests.py
@@ -135,6 +135,7 @@ from .models import (
UnchangeableObject,
UndeletableObject,
UnorderedObject,
+ UserMessenger,
UserProxy,
Villain,
Vodcast,
@@ -6873,6 +6874,32 @@ class SeleniumTests(AdminSeleniumTestCase):
name_input_value = name_input.get_attribute("value")
self.assertEqual(name_input_value, "Test section 1")
+ @screenshot_cases(["desktop_size", "mobile_size", "rtl", "dark", "high_contrast"])
+ @override_settings(MESSAGE_LEVEL=10)
+ def test_messages(self):
+ from selenium.webdriver.common.by import By
+ from selenium.webdriver.support.ui import Select
+
+ self.admin_login(
+ username="super", password="secret", login_url=reverse("admin:index")
+ )
+ UserMessenger.objects.create()
+ for level in ["warning", "info", "error", "success", "debug"]:
+ self.selenium.get(
+ self.live_server_url + reverse("admin:admin_views_usermessenger_changelist"),
+ )
+ checkbox = self.selenium.find_element(
+ By.CSS_SELECTOR, "tr input.action-select"
+ )
+ checkbox.click()
+ Select(self.selenium.find_element(By.NAME, "action")).select_by_value(
+ f"message_{level}"
+ )
+ self.selenium.find_element(By.XPATH, '//button[text()="Run"]').click()
+ message = self.selenium.find_element(By.CSS_SELECTOR, "ul.messagelist li")
+ self.assertEqual(message.get_attribute("innerText"), f"Test {level}")
+ self.take_screenshot(level)
+This gives screenshots for each of the messsages in different view modes. There appears to be quite a few issues accessibility wise for the messages component (contrast might need checking but also the RTL mode has the icon in the wrong place, the high contrast mode probably wants a border etc)
I think the @django/accessibility team might be able to check the component and give a list of improvements we might want to tackle in a separate commit/ticket
| @@ -2,32 +2,35 @@ | |||
| :root { | |||
| --primary: #264b5d; | |||
| --primary-fg: #f7f7f7; | |||
|
|
|||
There was a problem hiding this comment.
Please revert any changes of spaces
There was a problem hiding this comment.
Reverted, but I have to ask - it looks like the closing curly brace being not indented properly and the spaces on empty lines are the result of not formatting the css file, wouldn't it be good to fix the formatting there, or maybe in another ticket?
| @@ -341,7 +341,7 @@ input[type="submit"], button { | |||
| background-position: 30px 12px; | |||
| } | |||
|
|
|||
| ul.messagelist li.error { | |||
| ul.messagelist li.debug, ul.messagelist li.info, ul.messagelist li.error { | |||
There was a problem hiding this comment.
It feels quite strange to me that we have this with what's above and a special case for the warning, would be nice if there was a way to clean this up
There was a problem hiding this comment.
This is probably caused by the fact that the yes/no icons have a bit of padding in the svg itself, while the alert icon does not have it. Adding the padding to the alert svg should make it possible to simplify the css. What do you think about that?
| @@ -0,0 +1,3 @@ | |||
| <svg width="13" height="13" viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg"> | |||
| <path fill="#adadad" d="M896 1590.4c112 0 201.6-37.33 283.73-119.47 74.67-74.67 119.47-171.73 119.47-283.73v-395.73c0-112-37.33-201.6-119.47-283.73-74.67-74.67-171.73-119.47-283.73-119.47s-201.6 52.27-283.73 126.93c-82.13 74.67-119.47 171.73-119.47 283.73v395.73c0 112 37.33 201.6 119.47 283.73 82.13 82.13 171.73 112 283.73 112ZM694.4 1291.73h395.73v-201.6h-395.73v201.6ZM694.4 896h395.73v-201.6h-395.73v201.6ZM896 1792c-104.53 0-209.07-29.87-298.67-82.13-89.6-52.27-164.27-126.93-216.53-216.53H97.07v-201.6h209.07c0-29.87-7.47-67.2-7.47-97.07v-97.07H97.07v-201.6h201.6v-97.07c0-29.87 7.47-67.2 7.47-97.07H97.07v-201.6h276.27c22.4-37.33 52.27-74.67 74.67-104.53 37.33-37.33 74.67-67.2 112-89.6l-164.27-164.27L537.6 0l216.53 216.53c44.8-14.93 97.07-22.4 141.87-22.4s97.07 7.47 141.87 22.4L1254.4 0l141.87 141.87-164.27 164.27c37.33 22.4 74.67 52.27 104.53 89.6 29.87 37.33 59.73 67.2 82.13 104.53h276.27v201.6h-209.07c7.47 29.87 7.47 67.2 7.47 97.07v97.07h201.6v201.6h-201.6v97.07c0 29.87 0 67.2-7.47 97.07h209.07v201.6h-276.27c-52.27 89.6-126.93 164.27-216.53 216.53-97.07 52.27-201.6 82.13-306.13 82.13z"/> | |||
There was a problem hiding this comment.
The debug icon contrast in light mode is not strong enough
There was a problem hiding this comment.
It might be hard to get the colors with enough contrast for the icons, I tried to match to proportions to fit with other messages, but even currently implemented ones do not really meet the requirements.
The color has to be good enough for both light and dark mode, as the icon color does not change. I can see changing it so it uses the CSS variable, which would allow control over the icon color and change it between the themes, maybe in another ticket?
| @@ -0,0 +1,3 @@ | |||
| <svg width="13" height="13" viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg"> | |||
| <path fill="#53a7e0" d="M1561 510.5c-68.67-117.67-161.83-210.83-279.5-279.5-117.67-68.67-246.17-103-385.5-103s-267.83 34.33-385.5 103c-117.67 68.67-210.83 161.83-279.5 279.5-68.67 117.67-103 246.17-103 385.5s34.33 267.83 103 385.5c68.67 117.67 161.83 210.83 279.5 279.5 117.67 68.67 246.17 103 385.5 103s267.83-34.33 385.5-103c117.67-68.67 210.83-161.83 279.5-279.5 68.67-117.67 103-246.17 103-385.5s-34.33-267.83-103-385.5ZM1000.34 1396.65h-208.68v-659.76h208.68v659.76ZM896 628.93c-64.5 0-116.79-52.29-116.79-116.79s52.29-116.79 116.79-116.79 116.79 52.29 116.79 116.79-52.29 116.79-116.79 116.79z"/> | |||
There was a problem hiding this comment.
The icon contrast is also not strong enough here
|
@sarahboyce Thank you for the review. The issues withh the icon positioning and high contrast mode seem to be unrelated to changes in this PR, but if you are open to it I can add the CSS necessary to fix the two problems you listed, this could be a part of the cleanup you mentioned in one of the comments. This is how it could look like: |
05d5cd7 to
c226872
Compare
Trac ticket number
ticket-36386
Branch description
This PR adds missing style for messages in admin site with INFO and DEBUG levels. The colors are adjusted for both light and dark theme and pass the requirements for WCAG AA for contrast.
Checklist
mainbranch.