-
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
Features/answers visualization #14
Changes from 10 commits
18cb375
ad0c29f
4418329
3ce032d
057438d
4c6706d
47f4550
0193fcb
77a93c3
5b06954
ee50c94
3b7df0e
b2a6cbf
63aef88
eb01a01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import { memo, useMemo } from "react"; | ||
import { Bar, Doughnut } from "react-chartjs-2"; | ||
|
||
interface IAnswersChartProps { | ||
title?: string; | ||
options: string[]; | ||
selectedOptionsCount: number[]; | ||
} | ||
|
||
// docs for customisation: https://github.com/reactchartjs/react-chartjs-2 | ||
function UnmemoizedAnswersChart({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nice component. Could you write a story for this component? Also, some tests would be nice is there anything that is testable here? You can check the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more appropriate to move it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess it makes sense. Also, what do you think about renaming this component? I don't see anything that is specific to "answers". Since you are moving it to |
||
title, | ||
options, | ||
selectedOptionsCount | ||
}: IAnswersChartProps) { | ||
const data = useMemo(() => { | ||
return { | ||
labels: options, | ||
datasets: [ | ||
{ | ||
label: "# of answers", | ||
data: selectedOptionsCount, | ||
backgroundColor: [ | ||
"#ff638433", | ||
"#76f25d33", | ||
"#36a2eb33", | ||
"#ffce5633", | ||
"#765df233", | ||
"#21fcf233", | ||
"#ff9f4033" | ||
], | ||
borderColor: [ | ||
"#ff6384", | ||
"#76f25d", | ||
"#36a2eb", | ||
"#ffce56", | ||
"#765df2", | ||
"#21fcf2", | ||
"#ff9f40" | ||
], | ||
borderWidth: 2 | ||
} | ||
] | ||
}; | ||
}, [options, selectedOptionsCount]); | ||
|
||
return ( | ||
<div className="flex flex-cols align-center justify-center"> | ||
<div style={{ maxHeight: 300 }}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and the |
||
<Doughnut data={data} /> | ||
</div> | ||
|
||
<div style={{ width: 500 }}> | ||
<Bar data={data} options={{ maintainAspectRatio: false }} /> | ||
</div> | ||
</div> | ||
); | ||
} | ||
|
||
export const AnswersChart = memo(UnmemoizedAnswersChart); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know this. Thanks! Created issue #18 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good practice for Pure Components specially if they are costly to render There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's great, thanks for the reference! I've updated #18 with your reference |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
import { useEffect, useState } from "react"; | ||
import { useTranslation } from "react-i18next"; | ||
import { Link } from "react-router-dom"; | ||
import Button from "../../components/Button"; | ||
import Container from "../../components/Container"; | ||
import { useUser } from "../../contexts/user-context"; | ||
import { dynamicPathname } from "../../helpers"; | ||
import { routes } from "../../routes"; | ||
import { useSummaryListForExercise } from "../../services/exercises"; | ||
|
||
interface ExerciseDetailsProps { | ||
|
@@ -16,41 +18,52 @@ const ExerciseDetails = ({ offering, slug }: ExerciseDetailsProps) => { | |
const { | ||
summaryList, | ||
loading, | ||
refresh: refreshSummaryList, | ||
refresh: refreshSummaryList | ||
} = useSummaryListForExercise(offering, slug, token); | ||
|
||
const { t } = useTranslation(); | ||
|
||
const [numSubmissions, setNumSubmissions] = useState<number>(0); | ||
const [numUniqueUsers, setNumUniqueUsers] = useState<number>(0); | ||
const [lastRefresh, setLastRefresh] = useState<Date>(); | ||
|
||
useEffect(() => { | ||
if (loading) return; | ||
setNumUniqueUsers(summaryList.length); | ||
setNumSubmissions( | ||
summaryList | ||
.map((answerSummary) => answerSummary.answer_count) | ||
.reduce((a, b) => a + b, 0), | ||
.reduce((a, b) => a + b, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if you erased this manually or if it was some formatter, but this is the reason we keep the trailing commas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was done by vscode formatter, but I've just set it to allow trailing commas |
||
); | ||
setLastRefresh(new Date()); | ||
}, [loading, summaryList]); | ||
|
||
const answersRoute = dynamicPathname(routes.EXERCISE_ANSWERS, { | ||
offering, | ||
slug | ||
}); | ||
|
||
return ( | ||
<Container className="bg-gray-100 p-4 my-4 flex"> | ||
<div className="bg-gray-100 p-4 flex flex-col"> | ||
<div className="flex-grow"> | ||
<p> {slug} </p> | ||
{!!lastRefresh && ( | ||
<p> | ||
{t("submissions sent before", { | ||
total: numSubmissions, | ||
users: numUniqueUsers, | ||
lastUpdate: lastRefresh?.toLocaleString(), | ||
lastUpdate: lastRefresh?.toLocaleString() | ||
})} | ||
</p> | ||
)} | ||
</div> | ||
<Button onClick={refreshSummaryList}>{t("Reload")}</Button> | ||
</Container> | ||
<div className="flex flex-row mt-4 justify-around"> | ||
<Link to={answersRoute}> | ||
<Button>{t("See details")}</Button> | ||
</Link> | ||
<Button onClick={refreshSummaryList}>{t("Reload")}</Button> | ||
</div> | ||
</div> | ||
); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* | ||
* @param route Dynamic route. Ex: | ||
* ```typescript | ||
* "/api/offerings/:off_pk/exercises/:ex_slug/answers/" | ||
* ``` | ||
* @param params Object with dynamic params. Ex: | ||
* ```typescript | ||
* {off_pk: 1, ex_slug: 'quiz-0'}) | ||
* ``` | ||
* @returns Properly filled route | ||
* ```typescript | ||
* "/api/offerings/1/exercises/quiz-0/answers" | ||
* ``` | ||
*/ | ||
export function dynamicPathname(route: string, params: Record<string, string|number>) { | ||
for (let [key, value] of Object.entries(params)) { | ||
const regexp = new RegExp(`:${key}`, "g"); | ||
route = route.replace(regexp, String(value)); | ||
} | ||
return route; | ||
} | ||
toshikurauchi marked this conversation as resolved.
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.
Great! This should be a naming convention. I've created issue #16 to fix the previous code.