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

Assessments: Add Multiple Choice Answer Table Component #22054

Merged
merged 11 commits into from Apr 26, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Apr 25, 2018

This PR is to create a table thats shows the assessment overview for a class.
Step 1: create a multiple choice answer table react component

  • Display Questions (add sorting icon)
  • Display Multiple choice answer options A & B

Step 2: Add to storybook

assessment-table-rev

Next step:
-Add text prop (percent of students who select the correct answer) and correct answer icon (green check mark circle that is visible when the answer is correct).

@caleybrock
Copy link
Contributor

Looks like you'll need to merge staging into this branch:

bin/circle/merge-base:32:in `<main>': .circleci/config.yml has changed. Please merge the origin/staging branch into your branch and try again. (RuntimeError)
Exited with code 1

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

Looking good overall! Just a few naming suggestions.

I really liked that you defined your prop shape, it makes it much easier to understand what the component expects.

answerOptionTwo: PropTypes.string,
});

class MultipleChoiceAnswerTable extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this is a bit confusing. It'd expect it to be a table that shows me the answers, but really it shows me an overview of my students results. Maybe MultipleChoiceOverviewTable?

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 for the suggestion.

{
id: 2,
question: 'What is a 4-bit number for the decimal number Ten(10)',
answerOptionOne: '40%',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It'd be nice to have different percent breakdowns for each to show that the table works with all different values.

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 point!

...tableLayoutStyles.headerCell,
width: 90,
}},
transforms: [sortable],
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec doesn't actually show any of the columns as sortable. Did Poorva ask for a change? I'm guessing we might only want the question column sortable for now?


export const COLUMNS = {
QUESTION: 0,
A: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to call these answerOptionOne and answerOptionTwo since that's how you refer to them in code. A and B seem to be more of display strings.

const questionAnswerDataPropType = PropTypes.shape({
id: PropTypes.number.isRequired,
question: PropTypes.string,
answerOptionOne: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

With this name it seems like this will be A. The best answer.
Can this be someone that makes it more obvious that it's a percent, like percentAnsweredOption1?

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@nkiruka nkiruka merged commit b8eadef into staging Apr 26, 2018
@nkiruka nkiruka deleted the assessments-table branch April 26, 2018 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants