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

[#12283] Instructor view session results (course-wide): Add separate button to download results by question #12591

Merged
merged 13 commits into from
Sep 29, 2023

Conversation

MaharshiPedu
Copy link
Contributor

Fixes #12283

Outline of Solution

I have added a button that downloads question results on the right side near expand tab button.

image

I have used the same design that 👇🏽 follows.
image

@cedricongjh cedricongjh self-requested a review September 21, 2023 15:30
@cedricongjh cedricongjh added the s.ToReview The PR is waiting for review(s) label Sep 21, 2023
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

Hey @MaharshiPedu thank you for your PR!

Just a small thing, I think we should remove the download functionality from the question number

@MaharshiPedu
Copy link
Contributor Author

Thanks for your review. I have made the changes you suggested. Please have a look.

image

@jasonqiu212
Copy link
Contributor

jasonqiu212 commented Sep 24, 2023

Personally, the color contrast between the btn-primary download button and the bg-info panel does not sit right with me. I think it differs from the color contrast of the action buttons of other existing panels, for instance this one as mentioned:

Upon further inspection of the example above, I realized that the background color of btn-secondary was customized to be 10% darker than usual, creating the subtle difference in colors.

  • Customization happens in styles.scss line 164

To maintain consistency with existing dropdown panels, I tested out darkening the background color of btn-info by 3%:

Screen Shot 2023-09-23 at 9 19 33 PM

Why 3%?

  • btn-primary was tweaked to be darkened by 5% while btn-secondary was darkened by 10%, for aesthetic reasons I'm assuming. Thus, I believe consistency in the darkening percentage is not the priority, and aesthetics should be prioritized.
  • I felt that darkening 3% for btn-info appeared the best
  • Since I'm using my own view on the aesthetic, I'm also open to better suggestions here (I'm not the best designer out there)! Feel free to try out other colorings in styles.scss line 164

@cedricongjh What do you think?

@MaharshiPedu
Copy link
Contributor Author

I think you are right about the contrast between the btn-primary download button and the bg-info panel. Also, darkening btn-info by 3% does seem like a sweet spot aesthetically.

Suggestion:

If we look at consistency, then the chevron should also be of the same color as the other text inside the panel like in the ones below:

image

image

So, the chevron in the questions panel should also be black as the text inside that panel is black, like so:

image

@cedricongjh
Copy link
Contributor

Yup I agree with the points raised here! Good catch @jasonqiu212 and @MaharshiPedu.

Another thing I would like to suggest is that we should change the text from "Download" to "Download Results" as this will probably make it clearer to the user what the button is for before they mouse over it (and will then see the tooltip), what do you all think?

@jasonqiu212
Copy link
Contributor

@cedricongjh I agree! To keep it consistent with the other panels, I think changing the text to "Download Results" and removing the tool tip is a great idea.

@cedricongjh
Copy link
Contributor

all looks good, just one small nit:

As per your suggestion, let's change the chevron to black as well, same as the text color
image

@MaharshiPedu
Copy link
Contributor Author

MaharshiPedu commented Sep 26, 2023

@cedricongjh @jasonqiu212
I changed the color of chevron to black by adding a new @Input called chevronColor (with a default value of white color) in panel-chevron.component.ts. Then I passed black value of chevronColor from instructor-session-result-question-view.component.html.

Now, I am having trouble finding out the reason for the failing component tests. Would highly appreciate any help.

@jasonqiu212
Copy link
Contributor

Hi @MaharshiPedu, I believe some of the snapshot tests are failing after the chevron color change.

To update to update the snapshot tests, run npm run test and press a to run all test cases. After that, check through the snapshots to make sure the changes are as expected, then press u to update them. You can find more details on snapshot testing here.

@MaharshiPedu
Copy link
Contributor Author

I have updated the snapshots. Please have a look.

Copy link
Contributor

@cedricongjh cedricongjh left a 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 work on this

@cedricongjh cedricongjh added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Sep 27, 2023
Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment

Choose a reason for hiding this comment

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

@MaharshiPedu Just one more small nit!

@jasonqiu212
Copy link
Contributor

jasonqiu212 commented Sep 28, 2023

@cedricongjh Upon further inspection, after this merge, the input shouldShowDownloadQuestionResult in question-text-with-info.component.ts is no longer used anywhere. The input toggles the tooltip download feature on the question number.

Since the tooltip download feature is not UI friendly, do you think we should just remove the input and feature altogether? I think all future features to download something should be only done through an explicit button.

@cedricongjh
Copy link
Contributor

@jasonqiu212 i agree! let's create a new issue to remove the code completely, or if @MaharshiPedu likes, can remove it as part of this PR as well

@MaharshiPedu
Copy link
Contributor Author

I agree! I could do these changes as part of this PR.

Copy link
Contributor

@jasonqiu212 jasonqiu212 left a 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!

@jasonqiu212 jasonqiu212 merged commit 2b9f0e5 into TEAMMATES:master Sep 29, 2023
8 checks passed
@wkurniawan07 wkurniawan07 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 Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V8.30.0 milestone Jan 21, 2024
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 view session results (course-wide): Add separate button to download results by question
5 participants