-
Notifications
You must be signed in to change notification settings - Fork 15
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
Simplify edit series form #1021
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.
Neat. This makes more sense, especially after putting the description in the box! Just two small things.
<div> | ||
<Heading>{t("series.settings.order")}</Heading> | ||
<DisplayOptionGroup type="radio" optionProps={orderProps} {...{ form }} /> | ||
</div> | ||
<div> | ||
<Heading>{t("manage.realm.content.series.layout.heading")}</Heading> | ||
<DisplayOptionGroup type="checkbox" optionProps={layoutProps} {...{ form }} /> | ||
</div> |
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.
Isn't there a grouping necessary for accessibility? The radio buttons and checkboxes are not part of a fieldset. I would expect some aria-label as described in Approach 2
or a fieldset.
https://www.w3.org/WAI/tutorials/forms/grouping/
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.
Hm I think you are right, thank you for pointing it out.
This refactors the layout/order option inputs of series and video add/edit forms and simplifies the edit series buttons into regular radio/checkbox inputs.
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.
LGTM!
I will merge this now already. If Gregors comment was not properly addressed, please let us know. But this can be addressed later, this PR didn't make anything worse in that regard.
<LayoutChooser {...{ currentLayout, form }} /> | ||
<div | ||
role="group" | ||
aria-labelledby={t("manage.realm.content.display-options")} |
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.
Could it be better to have a group for each DisplayOptionGroup? I tried it with the Fedora screen reader, which added context about the selected group and it read Panel Reihenfolge
instead of h3 Reihenfolge
. Or is that too verbose, and display options is enough?
aria-labelledby
should be replaced with aria-label
:
aria-labelledby={t("manage.realm.content.display-options")} | |
aria-label={t("manage.realm.content.display-options")} |
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, right again. Sorry for missing this, we have a lot going on right now. Would you be open to adjust that accordingly in #1058?
Regarding groups for each display option, I'm not sure what works best for screenreader users.
I'm testing with VoiceOver on macOS, which indicates that I am entering and exiting the group, and for the individual panels only reads their heading. I feel like that is enough? But I wouldn't mind trying what you suggested - could you maybe include that in your PR as well?
As discussed in PR elan-ev#1021 this is a PoC to test screenreader output with individual groups.
As discussed in PR #1021 this is a PoC to test screenreader output with individual groups.
This refactors the layout/order option inputs of series and video add/edit forms and simplifies the edit series buttons into regular radio/checkbox inputs.
Closes #1009