Features/answers visualization #14
Conversation
…stead of requesting all answers
…pport-api into features/answers-visualization
| summary?: ISummary; | ||
| long_answer?: string; |
There was a problem hiding this comment.
what's the difference between summary and long_answer when it's not a multiple choices question?
There was a problem hiding this comment.
In code questions, for example, long_answer includes the students code while summary has the number of passing tests and some other things about the result (don't really remember hehe).
I don't know where is the best place to discuss this, but I'm thinking that maybe it would be better to have input and result attributes instead of summary and long_answer. What do you think @igordsm?
| <Link | ||
| to={routes.INSTRUCTOR_HOME} | ||
| className="bg-gray-100 text-blue-500 px-4 py-2"> | ||
| <b>{t("Go back")}</b> |
There was a problem hiding this comment.
Why do we need a back button?
There was a problem hiding this comment.
Igor gave me a layout suggestion, so I think this button will probably be gone in the next PR
| import { memo, useMemo } from "react"; | ||
| import { Bar, Doughnut } from "react-chartjs-2"; | ||
|
|
||
| interface IAnswersChartProps { |
There was a problem hiding this comment.
Great! This should be a naming convention. I've created issue #16 to fix the previous code.
| )} | ||
|
|
||
| {!!textAnswers.length && ( | ||
| <Container> |
There was a problem hiding this comment.
I think we can extract a component here. What do you think @cicerotcv?
| } | ||
|
|
||
| // docs for customisation: https://github.com/reactchartjs/react-chartjs-2 | ||
| function UnmemoizedAnswersChart({ |
There was a problem hiding this comment.
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 Button component for some examples.
There was a problem hiding this comment.
Would it be more appropriate to move it to /components?
There was a problem hiding this comment.
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 /components, what about picking a more generic name?
|
|
||
| return ( | ||
| <div className="flex flex-cols align-center justify-center"> | ||
| <div style={{ maxHeight: 300 }}> |
There was a problem hiding this comment.
This and the style in the div below should be based on the theme in some way. If none of tailwinds defaults work in this case you can define some extensions on tailwind.config.js.
| 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.
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.
It was done by vscode formatter, but I've just set it to allow trailing commas
| ); | ||
| } | ||
|
|
||
| export const AnswersChart = memo(UnmemoizedAnswersChart); |
There was a problem hiding this comment.
I didn't know this. Thanks! Created issue #18
There was a problem hiding this comment.
This is a good practice for Pure Components specially if they are costly to render
There was a problem hiding this comment.
That's great, thanks for the reference! I've updated #18 with your reference
| @@ -10,6 +10,7 @@ | |||
| "forceConsistentCasingInFileNames": true, | |||
| "noFallthroughCasesInSwitch": true, | |||
| "module": "esnext", | |||
| "downlevelIteration": true, | |||
Updates:
src/helpersto store useful functions;/src/services/routes;