Multiple choice #135
Multiple choice #135
Conversation
| } | ||
|
|
||
| markedForDeletion(choice) { | ||
| const { question } = this.props.item; |
There was a problem hiding this comment.
So is this modifying some object deep in the props?
# Conflicts: # client/js/_author/components/assessments/_edit_assessment.jsx # client/js/_author/components/assessments/_new_assessment.jsx # client/js/_author/components/assessments/assessment_form.jsx # client/js/_author/components/assessments/assessment_items.jsx # client/js/_author/components/assessments/question_types/_question.jsx
| }); | ||
| } | ||
|
|
||
| moveChoice(choice, up) { |
There was a problem hiding this comment.
Maybe have the options know their index, so you don't have to do the find stuff?
There was a problem hiding this comment.
and pass their index as a parameter to the move function
|
|
||
| const newIndex = up ? index - 1 : index + 1; | ||
|
|
||
| const earlierItem = choices[newIndex]; |
There was a problem hiding this comment.
Isn't this modifying the store without an action? Maybe rearrange a clone of these?
| @@ -80,29 +77,18 @@ export class EditAssessment extends React.Component { | |||
| ); | |||
| } | |||
|
|
|||
| updateAssessment() { | |||
| updateAssessment(newFields) { | |||
| const { assessment } = this.props; | |||
| this.props.updateAssessment( | |||
| this.props.params.bankId, | |||
There was a problem hiding this comment.
Would it make sense to merge javascript objects here instead of merging each property on the object manually? Then it might be more future proof if you ever need to add a property other than those 3.
| this.props.updateItem( | ||
| { | ||
| id, | ||
| name: newItemProperties.name || displayName.text, | ||
| description: newItemProperties.description || description.text, | ||
| question: { |
There was a problem hiding this comment.
I think it might make sense to merge objects here as well.
# Conflicts: # client/js/_author/components/assessments/assessment_items.jsx # client/js/_author/components/assessments/question_types/_question.jsx # client/js/_author/components/assessments/question_types/multiple_choice.jsx
# Conflicts: # client/js/_author/components/assessments/_edit_assessment.jsx # client/js/_author/components/assessments/assessment_form.jsx # client/js/_author/components/assessments/assessment_items.jsx # client/js/_author/components/assessments/question_types/_question.jsx
| description: this.state.assessment.description, | ||
| id: this.props.params.id, | ||
| }, | ||
| { ...{ id: this.props.params.id }, ...newFields }, |
There was a problem hiding this comment.
why bother with ...{ id: this.props.params.id }, why not just { id: this.props.params.id, ...newFields }
| }; | ||
|
|
||
| this.props.updateItem(newItem); | ||
| this.props.updateItem({ ...{ id: item.id }, ...newItemProperties }); |
There was a problem hiding this comment.
Same here, why bother splaying a single key?
No description provided.