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.

Sign up for GitHub

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

Jump to bottom

Assessments: Overview table - calculate not answered value #22156

Merged
merged 21 commits into from
May 14, 2018
Merged

Assessments: Overview table - calculate not answered value #22156

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
230227c
add function to calculate notAnswered value
nkiruka May 2, 2018
4fb8dc9
add notAnswered prop
nkiruka May 2, 2018
d231ca4
update multiple choice data
nkiruka May 2, 2018
f7369ce
add notAnswered prop
nkiruka May 2, 2018
549baec
Add cell formatter to notAnswered
nkiruka May 2, 2018
f35273f
remove not Answered prop
nkiruka May 3, 2018
3ec3594
update code per PR review
nkiruka May 3, 2018
948b52f
update code per Pr
nkiruka May 4, 2018
e6c4526
add multiple choice option
nkiruka May 4, 2018
3260af7
update code per PR review
nkiruka May 4, 2018
47b70ee
Add default props
nkiruka May 4, 2018
c547f58
refactor code
nkiruka May 4, 2018
fc904b1
merge conflict
nkiruka May 4, 2018
9fae7d1
code clean
nkiruka May 4, 2018
e7d313f
code clean
nkiruka May 4, 2018
7374363
update code per PR review
nkiruka May 8, 2018
afc67e8
update code per PR review
nkiruka May 8, 2018
a48213f
remove unused data columns array
nkiruka May 8, 2018
49374e1
change percentAnswered proptype
nkiruka May 8, 2018
fd61e59
Add percentValue to props
nkiruka May 11, 2018
3899573
fixed storybook error
nkiruka May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions apps/src/templates/sectionAssessments/MultipleChoiceAnswerCell.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const styles = {
color: color.level_perfect,
float: 'left',
width: 20,
fontSize: 25,
},
text: {
color: color.charcoal,
Expand All @@ -25,12 +24,13 @@ const styles = {
float: 'left',
paddingRight: 40,
width: 20,
textAlign: 'center',
},
};

class MultipleChoiceAnswerCell extends Component {
static propTypes = {
percentValue: PropTypes.string.isRequired,
percentValue: PropTypes.number.isRequired,
isCorrectAnswer: PropTypes.bool,
};

Expand All @@ -39,7 +39,12 @@ class MultipleChoiceAnswerCell extends Component {
return (
<div>
<div>
{percentValue}
{(percentValue >= 0) &&
<span>{`${percentValue}%`}</span>
}
{(percentValue < 0 ) &&
<span>{'-'}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you said Poorva likes it without -?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poorva also likes it with the dash and she is open to either option. I ran a mini survey with Maddie and Erin and the 'dash' option is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

}
</div>
<div>
{isCorrectAnswer &&
Expand All @@ -51,4 +56,9 @@ class MultipleChoiceAnswerCell extends Component {
}
}


MultipleChoiceAnswerCell.defaultProps = {
percentValue: -1
};

export default MultipleChoiceAnswerCell;
58 changes: 36 additions & 22 deletions apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,34 @@ export const COLUMNS = {
QUESTION: 0,
};

const alphabetMapper = [
commonMsg.answerOptionA(),
commonMsg.answerOptionB(),
commonMsg.answerOptionC(),
commonMsg.answerOptionD(),
commonMsg.answerOptionE(),
commonMsg.answerOptionF(),
commonMsg.answerOptionG(),
];
const NOT_ANSWERED = 'notAnswered';

const calculateNotAnswered = (multipleChoiceDataArr) => {
let total = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Thanks

multipleChoiceDataArr.forEach (studentsAnswersObj => {
if (studentsAnswersObj.percentAnswered) {
total += studentsAnswersObj.percentAnswered;
}
});

return (100 - total);
Copy link
Contributor

Choose a reason for hiding this comment

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

I asked "Do we have any multiple choice questions where students can select multiple answers, like a "select all that apply..." ?" in Slack because, if that's the case, you could run into a situation where calculateNotAnswered returns a negative number. For example, let's say there are only two answer choices: "A" and "B" and students are allowed to select: only "A", only "B", none, or "A" AND "B". If 100% of the students selected "A" AND "B", total in this function is 200, and you'd return -100. Let's confirm the requirements with Poorva.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Since this is an overview table of all the students' assessments, I do not think this will be an issue. For the individual student tables, this will be applicable (a good test :-) to check). Thoughts!

Copy link
Contributor

Choose a reason for hiding this comment

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

If students can select multiple answer options, I'm still concerned about calculateNotAnswered for this overview table. For example we could have a scenario like:
img_0564
In this case we would want the % not answered to be 25% because 1 out of 4 students did not answer the question; however the function as written would return 0%.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a great unit test case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I concur!! Tests will be covered in the next PR.

};

const answerColumnsFormatter = (percentAnswered, {rowData, columnIndex, rowIndex, property}) => {
const cell = rowData.answers[columnIndex - 1];

let percentValue = 0;

if (property === 'notAnswered') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use your constant here instead of this string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, Thanks

percentValue = calculateNotAnswered(rowData.answers);
} else {
percentValue = (cell && cell.percentAnswered);
}

return (
<MultipleChoiceAnswerCell
id={rowData.id}
percentValue={(cell && `${cell.percentAnswered}%`) || '-'}
percentValue={percentValue}
isCorrectAnswer={cell && cell.isCorrectAnswer}
/>
);
Expand All @@ -36,7 +48,6 @@ const questionAnswerDataPropType = PropTypes.shape({
question: PropTypes.string,
percentAnswered: PropTypes.string,
isCorrectAnswer: PropTypes.bool,
notAnswered: PropTypes.string,
});

class MultipleChoiceOverviewTable extends Component {
Expand Down Expand Up @@ -78,16 +89,17 @@ class MultipleChoiceOverviewTable extends Component {
props: {style: tableLayoutStyles.headerCell},
},
cell: {
format: answerColumnsFormatter,
props: {style: tableLayoutStyles.cell},
}
}
);

getAnswerColumn = (index) => (
getAnswerColumn = (columnLabel) => (
{
property: 'percentAnswered',
header: {
label: alphabetMapper[index],
label: columnLabel,
props: {style: tableLayoutStyles.headerCell},
},
cell: {
Expand All @@ -112,18 +124,20 @@ class MultipleChoiceOverviewTable extends Component {
);

getColumns = (sortable) => {
const maxAnswerChoicesLength = this.props.questionAnswerData.reduce((answersTotal, currentAnswerCount) => {
return Math.max(answersTotal, currentAnswerCount.answers.length);
}, 0);

let dataColumns = [];
Copy link
Contributor

@caleybrock caleybrock May 4, 2018 •
edited

Choose a reason for hiding this comment

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

I'm not sure I totally understand the reason for the re-factoring in this function but I do have some suggestions to make it easier to read.
Your new code creates an empty dataColumns, pushes one element to it, and then deconstructs it to add it to an array. Would it be simpler to just add this.getQuestionColumn(sortable) to your array like this?

return [
  this.getQuestionColumn(sortable),
  ...columnLabelNames,
  this.getNotAnsweredColumn()},
];

I think it could make your final constructed array easier to read.
Note: you could make this.getNotAnsweredColumn() return an object that already has the property NOT_ANSWERED so you don't have to reconstruct it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it does make it easier to read. Fixed.


dataColumns.push(this.getQuestionColumn(sortable));
Copy link
Contributor

Choose a reason for hiding this comment

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

dataColumns is not longer used - so you can delete it.

for (let i = 0; i < maxAnswerChoicesLength; i++) {
dataColumns.push(this.getAnswerColumn(i));
}
dataColumns.push(this.getNotAnsweredColumn());

return dataColumns;
const maxOptionsQuestion = [...this.props.questionAnswerData].sort((question1, question2) => (
question1.answers.length - question2.answers.length
)).pop();

let columnLabelNames = maxOptionsQuestion.answers.map((answer) => {
return this.getAnswerColumn(answer.multipleChoiceOption);
});

return [...dataColumns, ...columnLabelNames, ...[{property: NOT_ANSWERED, ...this.getNotAnsweredColumn()}]];

};

render() {
Expand Down
45 changes: 23 additions & 22 deletions apps/src/templates/sectionAssessments/MultipleChoiceOverviewTable.story.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,48 @@
import React from 'react';
import MultipleChoiceOverviewTable from './MultipleChoiceOverviewTable';
import commonMsg from '@cdo/locale';

const multipleChoiceData = [
{
id: 1,
question: '1. What is a variable?',
answers: [{percentAnswered: 40, isCorrectAnswer: true},
{percentAnswered: 20, isCorrectAnswer: false},
{percentAnswered: 20, isCorrectAnswer: false},
{percentAnswered: 20, isCorrectAnswer: false},
answers: [{multipleChoiceOption: commonMsg.answerOptionA(), percentAnswered: 40, isCorrectAnswer: true},
{multipleChoiceOption: commonMsg.answerOptionB(), percentAnswered: 20, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionC(), percentAnswered: 20, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionD(), percentAnswered: 20, isCorrectAnswer: false},
],
Copy link
Contributor

@Erin007 Erin007 May 2, 2018 •
edited

Choose a reason for hiding this comment

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

maybe I'm confused, but I'm a little concerned that we're relying on order to match percentAnswered to specific answer options (eg. "A"). What if we had something like {multipleChoiceOption: "A", percentAnswered: 40, isCorrectAnswer: true}? Then all of the relevant data gets bundled together.

Copy link
Contributor

@Erin007 Erin007 May 2, 2018 •
edited

Choose a reason for hiding this comment

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

If you want the translatable string, I think you could do:
{multipleChoiceOption: {i18n.answerOptionA()}, percentAnswered: 40, isCorrectAnswer: true}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Erin007 This is a valid point! After our in-person discussion, there is a tremendous payoff being able to identify which percentAnswered is associated with the answer choice. Additionally, debugging is easier. Thanks!

},
{
id: 2,
question: '2. What is a 4-bit number for the decimal number Ten(10)?',
answers: [{percentAnswered: 30, isCorrectAnswer: false},
{percentAnswered: 10, isCorrectAnswer: true},
{percentAnswered: 10, isCorrectAnswer: false},
{percentAnswered: 10, isCorrectAnswer: false},
{percentAnswered: 20, isCorrectAnswer: false},
{percentAnswered: 10, isCorrectAnswer: false},
answers: [{multipleChoiceOption: commonMsg.answerOptionA(), percentAnswered: 30, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionB(), percentAnswered: 10, isCorrectAnswer: true},
{multipleChoiceOption: commonMsg.answerOptionC(), percentAnswered: 10, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionD(), percentAnswered: 10, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionE(), percentAnswered: 20, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionF(), percentAnswered: 10, isCorrectAnswer: false},
],
},
{
id: 3,
question: '3. What is the minimum number of bits you will need to encode the 26 letters of the alphabet?',
answers: [{percentAnswered: 50, isCorrectAnswer: false},
{percentAnswered: 15, isCorrectAnswer: false},
{percentAnswered: 20, isCorrectAnswer: true},
{percentAnswered: 5, isCorrectAnswer: false},
{percentAnswered: 5, isCorrectAnswer: false},
answers: [{multipleChoiceOption: commonMsg.answerOptionA(), percentAnswered: 50, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionB(), percentAnswered: 15, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionC(), percentAnswered: 20, isCorrectAnswer: true},
{multipleChoiceOption: commonMsg.answerOptionD(), percentAnswered: 5, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionE(), percentAnswered: 5, isCorrectAnswer: false},
],
},
{
id: 4,
question: '4. What is a function?',
answers: [{percentAnswered: 15, isCorrectAnswer: false},
{percentAnswered: 18, isCorrectAnswer: false},
{percentAnswered: 10, isCorrectAnswer: false},
{percentAnswered: 9, isCorrectAnswer: false},
{percentAnswered: 5, isCorrectAnswer: false},
{percentAnswered: 45, isCorrectAnswer: true},
{percentAnswered: 5, isCorrectAnswer: false},
answers: [{multipleChoiceOption: commonMsg.answerOptionA(), percentAnswered: 15, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionB(), percentAnswered: 18, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionC(), percentAnswered: 10, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionD(), percentAnswered: 9, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionE(), percentAnswered: 5, isCorrectAnswer: false},
{multipleChoiceOption: commonMsg.answerOptionF(), percentAnswered: 32, isCorrectAnswer: true},
{multipleChoiceOption: commonMsg.answerOptionG(), percentAnswered: 5, isCorrectAnswer: false},
],
},
];
Expand Down