Skip to content
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

[#12535] Instructor Course Page: Make "Show" group tabbable #12540

Merged
merged 15 commits into from
Aug 17, 2023

Conversation

rexong
Copy link
Contributor

@rexong rexong commented Aug 1, 2023

Fixes #12535

Outline of Solution

I have added attributes tabindex="0" so that user can use the tab button to focus on the Show anchor tag.
Then, I added (keyup.enter) so that when the user pressed enter it will render the statistic.

In addition, I noticed that when hovering over the Show anchor tag with the cursor, the cursor is not a pointer. As such, I have added a CSS clickable to make the cursor a pointer when hovering over the anchor tag.

Here is a video demo

TEAMMATES.-.Online.Peer.Feedback_Evaluation.System.for.Student.Team.Projects.-.Google.Chrome.2023-08-01.13-35-04.mp4

@rexong
Copy link
Contributor Author

rexong commented Aug 1, 2023

I may have complicated my solution by using tabindex and keyup.enter. I have updated the solution and removed the square bracket for tmRouterLink and assign its current route. This allows user to tab through the show anchor tag and also allow them to press enter to render the statistics.

@weiquu
Copy link
Contributor

weiquu commented Aug 2, 2023

Thanks @rexong! Is the clickable CSS class still relevant? I removed it from the HTML and still saw a pointer on hover.

Also, when using a screen reader, the element is being read as a "visited link" - not sure if that's appropriate in this instance. I would suggest adding role="button" to each of the elements; then, the screen reader will simply tell the user that it is a button, removing the confusion of "visited" and "link". (A button also seems more similar to what the "Show" element does, which is a clickable element that triggers a response when activated by the user)

@rexong
Copy link
Contributor Author

rexong commented Aug 2, 2023

Hello @weiquu
I would have to double check on the clickable again.
Regarding the second point, would it be better if we change the tag from an <a> tag to a <button> tag instead?

@weiquu
Copy link
Contributor

weiquu commented Aug 2, 2023

Regarding the second point, would it be better if we change the tag from an <a> tag to a <button> tag instead?

Hmm not too sure about this... @ seniors (@zhaojj2209 @samuelfangjw @wkurniawan07) do you foresee any issues with the change from <a> to <button>? I think the "Show" element behaviour and functionality is more like a button, but am unclear as to why it was implemented as a link.

@rexong in any case, I think we can keep it as <a> for this PR, since any change will likely have to be implemented in other components as well that go beyond the scope of this issue

@samuelfangjw
Copy link
Member

Yes please do. As pointed out by @weiquu, element is wrongly tagged here.

I also have reservations about styling buttons as links, but might be acceptable in this case since it's existing behaviour.

@rexong
Copy link
Contributor Author

rexong commented Aug 2, 2023

Okay, I will make the change to button instead.

@rexong
Copy link
Contributor Author

rexong commented Aug 7, 2023

Ready for review!

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick comment, otherwise looks good!

Show
</a>
<button *ngIf="!course.isLoadingCourseStats; else loadingSpinner"
id="show-statistics-{{ i }}" (click)="getCourseStats(i)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I check if this id attribute should be here or for the previous column? I think it was previously on the "Teams" column... doubt it makes much of a difference though, but just in case, should we shift it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@weiquu weiquu added the s.ToReview The PR is waiting for review(s) label Aug 8, 2023
@weiquu weiquu requested a review from cedricongjh August 8, 2023 09:10
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@weiquu weiquu added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Aug 9, 2023
@rexong
Copy link
Contributor Author

rexong commented Aug 14, 2023

Hello, bumping this PR for final review!

@rexong
Copy link
Contributor Author

rexong commented Aug 17, 2023

Hello, bumping this PR for final review!

@cedricongjh cedricongjh merged commit 6a89e17 into TEAMMATES:master Aug 17, 2023
8 checks passed
@samuelfangjw samuelfangjw added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Aug 21, 2023
@samuelfangjw samuelfangjw added this to the V8.29.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructor Course Page: "Show" group cannot be tabbed over
5 participants