[#12290] Instructor view session results (course-wide): Improve display of no-response card header

[rhyses-pieces]

Fixes #12290

Outline of Solution

I reformatted the instructor-session-no-response-panel-component to have the .card class, and nested old code accordingly. The changes added include:

• changing the card-header from a <div> into a <button>
• adding the <tm-panel-chevron> to be the first element under the card-header button
• nesting the text "Participants who have not responded to any question" and card-header-btn-toolbar under a flex container
• adding flex properties to the card-header-btn-toolbar so that it behaves smoothly in mobile view.

Screenshot of the no-response card header in mobile view:

Comments

After reformatting the no-response card header to be consistent with the edit question card headers, I noticed that the no-response card header is inconsistent with the other card headers on the session results page:

Should the other card headers also be consistent with the edit question card headers and no-response card header too?

Please let me know if there's any questions or concerns. This is my first time contributing code to an open source project, so I'm open to feedback!

[weiquu]

Hi @rhyses-pieces, the approach generally looks fine, but do fix the failing tests (snapshot, accessibility, E2E) before we do a complete review.

After reformatting the no-response card header to be consistent with the edit question card headers, I noticed that the no-response card header is inconsistent with the other card headers on the session results page:

Can you help to check if the panel chevron is usually on the right or left in other pages? I think it would be good to standardise the chevron's location across the entire website. @zhaojj2209 do weigh in if you have any other opinions

[zhaojj2209]

@weiquu The positions are currently not standardised; the submit feedback and instructor session edit panels have the chevron on the left, the rest have it on the right. We can discuss whether to standardise the position in a separate issue; for this particular PR, let's keep it consistent with the previous design and have the chevron on the right.

[rhyses-pieces]

Hello! Thanks for the feedback.

I've updated the chevron's position in the no-response card header, but I wasn't sure how to resolve indicating that the card header is expandable without using <button> because there are other buttons within the card header. Should the card header stay a <div> in order for it to avoid the nested interactables issue?

Also, I'm running into problems with testing on my local system. My system keeps crashing even after following guidelines to set up E2E testing for the Edge browser. (For reference, I'm using Windows and WSL for my local dev environment.) Not sure how else to do testing on my end.

[zhaojj2209]

I've updated the chevron's position in the no-response card header, but I wasn't sure how to resolve indicating that the card header is expandable without using <button> because there are other buttons within the card header. Should the card header stay a <div> in order for it to avoid the nested interactables issue?

In this case I think it's fine to have nested buttons. Let's go with an approach similar to the other card headers (e.g. instructor edit session card header).

Also, I'm running into problems with testing on my local system. My system keeps crashing even after following guidelines to set up E2E testing for the Edge browser. (For reference, I'm using Windows and WSL for my local dev environment.) Not sure how else to do testing on my end.

Unfortunately E2E testing for TEAMMATES is known to be wonky on Edge, we recommend using other browsers such as Chrome or Firefox. If E2E tests still do not work on your local machine, you can use the E2E tests workflow on GitHub to debug as well, but we would not recommend doing it directly on this PR as repeated commits would spam maintainers with notifications. You can try using the following steps instead:

1. Enable GitHub actions on your own forked repo
2. Open a PR to the master branch of your own forked repo
3. Make changes until the E2E test workflow passes
4. Make the same fix to this PR after a fix has been found

[zhaojj2209]

@rhyses-pieces My bad, forgot that nested buttons will cause accessibility tests to fail. Let's revert the change, have checked and verified that the other card headers in the codebase still use divs.

[rhyses-pieces]

@zhaojj2209 Hello! Thanks for the feedback. I've fixed the nested buttons issue as suggested, and E2E testing is now passing.

[zhaojj2209]

LGTM, thanks for the changes!

[weiquu]

LGTM! Thanks for contributing