-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#12279] Instructor home page: Improve display of card header on mobile #12567
[#12279] Instructor home page: Improve display of card header on mobile #12567
Conversation
Change to the mobile look. Displaying the options under the course title.
remove border-0 class (included as div was changed to button)
Ready for review :D |
Hi @Mex7180, Thank you for your contribution to the project, and sorry for the late review! Here are my comments:
Thanks! |
Hi @jasonqiu212, I tried earlier to change the card header to a button element and tried again today. But I keep running into Issues with the Accessibility Tests failing, caused by nested interactivity (Test error: PS: The dropdown icon (panel-chevron) is now being displayed on the right. |
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
…ttps://github.com/Mex7180/teammates into TEAMMATES#12279-mobile-display-course-card-header
Hi @jasonqiu212 and @zhaojj2209 My changes
By changing the panel-chevron component to a button it can be selected with the keyboard, has a description and has the Let me know what you think. Snapshots: |
- change to button - add accessibility attributes
- card-header to div (nested interactivity) - remove invalid aria-attr.
Ready for review :D |
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.
Hi @Mex7180, Once again I apologize for the late review, and thank you for investigating into this issue!
I took a further look at #12396, and you are correct! My bad, I believe the PR I gave you for reference was merged before the accessibility test was fully included.
While #12396 passed the accessibility test by reverting the button
to div
, I noticed that the panel dropdown still was not accessible via tabbing and cannot be picked up by the screen reader.
Hence, I think your solution of converting the panel chevron to a button is genius and serves as an alternative to changing the entire header into a button, since:
- Users can toggle dropdown via tab and screen readers can now pick up the dropdown
- Passes accessibility test
- Preserves desktop functionality where entire panel is clickable
Thank you for coming up with this solution :)
There are some nits I would like to raise, because the panel chevron is used in many components. Let's ensure that changing the chevron to a button
does not affect the appearance in any of the pages!
@domlimm @cedricongjh Do you think this solution works to improve the accessibility of the dropdown panel? Hoping to look for more opinions on this fix :)
src/web/app/components/panel-chevron/panel-chevron.component.html
Outdated
Show resolved
Hide resolved
- set background color to inherit - change aria-label - update snapshots for testing
...nents/question-types/question-edit-answer-form/text-question-edit-answer-form.component.html
Outdated
Show resolved
Hide resolved
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.
Solution LGTM! (Do look into the removable of the aria label for the rich-text-editor though, as @jasonqiu212 mentioned)
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! Thank you for contributing to TEAMMATES!
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, thank you for the contribution!
Fixes #12279
Card-header-toolbar changed to display as flex-container on mobile screens (see screenshots below) and changed from div-element to button element for screenreader compatibility.
An other option would be with using grid to create a 2X2 grid with all 4 buttons. I also think the panel-chevron component (drop-down icon) could be positioned to the right instead of below.
Any opinions?
Screen-size of 450px
Screen-size of 320px
Screen-size of 768px