♿ [#2346] Remove spans, correct list-items #1172
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. |
cd8ce30 to
3399091
Compare
c7a5298 to
c6077f4
Compare
c58db10 to
1216031
Compare
|
Adding both @stevenbal and @pi-sigma as reviewers since you've worked with these pages and can check to see if the stying is still the same after these changes, in for example "Mijn afspraken" etc. |
There was a problem hiding this comment.
Looks good. Minor questions about the use of aria-label.
Also, I noticed that the link for the Mijn begeleider card in the profile page behaves differently from the link for Mijn contacten in case the user has no contacts. The latter leads to /mijn-profiel/contacts/ (even if there aren't any contacts), while the latter leads to mijn-profiel/# (instead of /mijn-profiel/contacts?type=begeleider ). I think this should be made consistent.
I would prefer if both lead to the contacts page. It makes sense to direct to the contacts page even if the user doesn't have any existing contacts or mentors, because they can create new ones.
It's an easy fix, but could also be addressed in a separate PR.
| {% if inbox_page_is_published %} | ||
| <span class="link link--icon link--secondary" aria-label="{% trans "Stuur een bericht" %}" title="{% trans "Stuur een bericht" %}"> | ||
| {% if inbox_page_is_published %} | ||
| <span class="link link--icon link--secondary" aria-label="{% trans "Stuur een bericht" %}" title="{% trans "Stuur een bericht" %}"> |
There was a problem hiding this comment.
Do we need the aria-label here, since we have "Stuur een bericht" immediately below?
There was a problem hiding this comment.
That is actually an issue that will come up with the other thirty accessibility tasks still to come, but I will correct it on the profiel page here anyway.
Important to note: aria-label does not work on static elements such as div or span; it is meant for interactive elements that need a label, like buttons.
(The OIP project is quite cluttered with them...).
@pi-sigma That issue is outside of the scope of accessibility so I put it in Taiga here: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2408 |
| @@ -1257,25 +1257,23 @@ def test_render_list_if_appointments_are_found(self, m): | |||
|
|
|||
| passport_appointment = PQ(cards[0]).find("ul").children() | |||
|
|
|||
| self.assertEqual(passport_appointment[0].text, "Aanvraag paspoort") | |||
There was a problem hiding this comment.
I think it would be good to keep this assertion if possible
There was a problem hiding this comment.
self.assertEqual(PQ(cards[0]).find(".card__heading-2").text(), "Aanvraag paspoort")
| self.assertEqual( | ||
| PQ(cards[0]).find("a").attr("href"), | ||
| f"{self.data.config.booking_base_url}{self.data.appointment_passport.publicId}", | ||
| ) | ||
|
|
||
| id_card_appointment = PQ(cards[1]).find("ul").children() | ||
|
|
||
| self.assertEqual(id_card_appointment[0].text, "Aanvraag ID kaart") |
There was a problem hiding this comment.
self.assertEqual(PQ(cards[1]).find(".card__heading-2").text(), "Aanvraag ID kaart")
issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2346
Turns out there were faulty Spans around LI's inside UL's in some Cards pages + also wrong use of paragraph tags in case of empty list-items. Also: Headings should not be a direct child inside UL's.