-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accessibility improvements for submission report pdf #5099
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we should have the translations in this PR. I remember we do it in the release flow (let me know if something has changed).
@@ -73,6 +74,15 @@ def __call__(self, component: ComponentT, value: Any) -> str: | |||
formatted_values = ( | |||
force_str(self.format(component, value)) for value in values | |||
) | |||
|
|||
# Check if formatted_values isn't empty | |||
formatted_values_copy, formatted_values = tee(formatted_values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used this before, it seems nice instead of a list I guess, but I do not know how it behaves concerning performance and efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the documentation again, i think it would be fine (this setup can be improved though..). It copies the formatted_values
iterable object, so we can check the content on the copy without moving through the original iterable.
But yeah, i'll have a look if i can create a solution using lists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, this can be greatly simplified with list.. Changing it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't say to change it. I just said that I haven't used it and I am not aware of how it works!If you think though that it should be changed to a list ok. I thought you have discussed this with Sergei and he suggested this
Ah yeah, you're right.. I'll remove them from the PR 👍 |
Added aria tags that showcase that field label and field value are related
…n report pdf To indicate when a field has been left empty by the user, show "No information provided" in the place where the filled in information would have been. This also helps screen reader users identify which fields aren't filled in.
Without the `spaceless` tags the pdf will be generated with leading and trailing whitespace. This whitespace only visible/noticeable when you look at the title in the pdf metadata.
ead9eb0
to
fef4e04
Compare
return ( | ||
format_html( | ||
"<ul>{values}</ul>", | ||
values=format_html_join( | ||
"", | ||
"<li>{}</li>", | ||
((selected_label,) for selected_label in selected_labels), | ||
), | ||
) | ||
if len(selected_labels) > 0 | ||
else "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codewise I think a guard clause would be more readable:
if not selected_labels:
return ""
return format_html(...)
this also makes it more easy to see where we're generating (potentially unsafe) HTML that needs to escape inputs.
@@ -76,19 +78,23 @@ <h2 class="subtitle">{{ submission_step_node.render }}</h2> | |||
{% if component_node.component.type == "fieldset" %} | |||
<span class="submission-step-row__fieldset-label">{{ component_node.label }}</span> | |||
{% else %} | |||
<div class="submission-step-row__label"> | |||
<span id="{{ component_node.key }}" class="submission-step-row__label"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if these are sufficiently unique if you use repeating groups, have you tested that?
Closes #5047
Changes
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh
./bin/compilemessages_js.sh
Dockerfile/scripts
./bin
folderCommit hygiene