-
Notifications
You must be signed in to change notification settings - Fork 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
cdeery/APER-2484/job select overflow scroll #10
Conversation
Need some feedback on the bump for Paragon to 20.42. This looks like a minor bump, but needs to update React to 18.2.15? Do we want to go there? |
@@ -27,7 +26,8 @@ const RelatedSkillsSelectableBoxSet = ({ jobSkillsList, selectedJobTitle, onChan | |||
type="radio" | |||
value={selectedJobTitle} | |||
onChange={onChange} | |||
columns={isExtraSmall ? 1 : 3} | |||
columns={3} | |||
className="overflow-scroll-medium" |
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 figma also mentions adding a label with "Select job title to see recommended courses". Is that an aria-label? Is that added to the SelectableBox?
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.
That would be a separate element. The Figma doesn't provide a mock up of how it should look but I imagine it would match the styles of the label for the first part of the quiz. This is how we do it for GoalSelect
: https://github.com/edx/frontend-app-skills/blob/main/src/skills-builder/skills-builder-modal/select-preferences/GoalSelect.jsx#L34-L38
So it would be an h4
with the text "Select job title to see recommended courses", however the Form.Label
that wraps the h4
will also need an ID. In order to associate that label with the SelectableBox.Set
, the Set will need an aria-labelledby
attribute with a value that matches the ID of the label.
FWIW, just using aria-label
is reserved for places where the element has no clearly accessible name (i.e. text/description) so that assistive technologies can still pick up on it.
All in all, it's not super trivial and may be out of scope for this ticket. What do you think?
EDIT: realizing also that the SelectableBox.Set
does not have a way to pass an aria-labelledby
attribute to the div
that is rendered as HTML. We'd have to update Paragon to employ the use of the aria-labelledby
attribute.
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.
That sounds non-trivial. Let's make another ticket
Where did you see that Paragon 20.42 requires React 18? If that's the case, we'd probably have to revert the Paragon update and just be okay with the warnings we were receiving for the SelectableBox component until we update React |
The CI test is failing with the message:
The only thing that changed was the Paragon version, so I assumed it was a Paragon requirement. |
So I pulled the branch and ran |
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!
Make the job selector not go single column on small screeens. instead, it is horizontally scrollable.