-
Notifications
You must be signed in to change notification settings - Fork 1
V2.0 #1
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
Conversation
src/Form/Widget.elm
Outdated
| -} | ||
| type alias Options = | ||
| { label : Maybe String |
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.
For accessibility reasons it would be best to keep label not Maybe String, but just String, and then have a Bool option to hide the label. Because if label is not visible - we still need to add it as aria-label for screen readers to show which field it is
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 added another property, ariaLabel : String, so that consumers of the library don't need to have any logic, and can just insert supplied attributes into HTML elements.
Co-authored-by: Maria Khovanskaya <[email protected]>
src/Form/Widget/Generate.elm
Outdated
| schema.title | ||
| |> Maybe.orElse (List.last scope |> Maybe.map UI.fieldNameToTitle) | ||
| |> Maybe.withDefault "" | ||
| |> Maybe.withDefault "unreachable" |
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.
Aria label must always be same as the label (if such is visible) or a descriptive label (if label is not visible / not present), so it makes sense to just have label and indication if that label is hidden (if label is hidden - we can use aria-label and if not - use label).
https://github.com/scrive/elm/blob/master/libs/scrive-ui/src/UI/Input.elm#L35
https://github.com/scrive/elm/blob/master/libs/scrive-ui/src/UI/Input.elm#L53
This currently will set some aria labels to "unreachable", which doesn't sound good for accessibility...
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.
Right, I misunderstood the purpose of aria-label a bit. I refactored the code to contain label : String and hideLabel : Bool.
The "unreachable" string is, in fact, never used -- it is unreachable, because scope is a non-empty list. Before, an empty string label ("") was used as a placeholder, but I thought it'd be better to make the failure more explicit.
Another fallback option would be to hide the whole widget, but that's more awkward in code and I'm not sure it's any better. I added a comment about it, do you think it's good enough?
-- `scope` is a non-empty list, so `fallback` is never "unreachable".... Or I can revert it into an empty string again and pretend there's no issue 😆 🤷
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.
The proper way would be to return a Maybe { label : String, hideLabel : Bool } I think, and then handle this situation whenever this function is called, but I will leave it up to you since it would require changing code that composes the control widget (you already return a maybe there, so indeed you could be not rendering the widget when an impossible state is reached).
But I think it is more issue-proof if we don't make impossible states possible with the unreachable placeholder - since there shouldn't be a widget without a label.
2.0.0