Skip to content

Conversation

remo5000
Copy link
Contributor

@remo5000 remo5000 commented Aug 3, 2018

Features

  • Only MCQ option are on the MCQChooser component
  • MCQ Questions are differentiated based on the presence of a solution (and hints):
    • Buttons have intent when the question has a solution
    • Hints are displayed as notifications when options are chosen
    • For ungraded questions, there is no intent on the buttons and there is no notification displayed.

@remo5000 remo5000 requested a review from ning-y August 3, 2018 08:31
* @param chosenOption the mcq option that is chosen in the state, i.e what should show up as "selected"
* @param solution the solution to the mcq, if any
*/
private getButtonIntent = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If possible could you review this with a bit more care? It looks ok to me, but feels like there might be come edge cases I haven't accounted for @ningyuansg

choices: [
{
content: 'A',
hint: 'hint A'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove hints for ungraded question. Inconsequential, but semantics matter.

margin-bottom: 20px;
.pt-card {
background-color: $cadet-color-3;
.mcq-options-parent {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nested some classes here, let me know if it's alright

@coveralls
Copy link

coveralls commented Aug 3, 2018

Pull Request Test Coverage Report for Build 547

  • 2 of 16 (12.5%) changed or added relevant lines in 1 file are covered.
  • 297 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.1%) to 25.379%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/workspace/MCQChooser.tsx 2 16 12.5%
Files with Coverage Reduction New Missed Lines %
src/actions/session.ts 1 86.36%
src/containers/academy/grading/GradingEditorContainer.ts 1 85.71%
src/actions/workspaces.ts 1 68.29%
src/components/workspace/MCQChooser.tsx 1 8.51%
src/containers/AnnouncementsContainer.ts 1 87.5%
src/reducers/states.ts 2 89.19%
src/components/academy/grading/GradingWorkspace.tsx 7 13.11%
src/components/assessment/index.tsx 8 78.3%
src/mocks/gradingAPI.ts 8 25.0%
src/components/academy/grading/index.tsx 11 31.58%
Totals Coverage Status
Change from base Build 515: -0.1%
Covered Lines: 846
Relevant Lines: 2821

💛 - Coveralls

Copy link
Member

@ning-y ning-y left a comment

Choose a reason for hiding this comment

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

  1. Consider if minimal={false} buttons for the options look better
  2. Space between options and the left/right borders of the card should be the same as the space between options and the top/bottom borders of the card
  3. Disable the highlight after click on options using CSS (refer to the end of _workspace.scss)

</div>
<div className="MCQChooser row">
<Card className="mcq-content-parent col-xs-12 middle-xs">
<div className="row mcq-options-parent between-xs">{options}</div>
Copy link
Member

Choose a reason for hiding this comment

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

Put in button group to avoid weird gaps between the options.

@ning-y ning-y merged commit c3145d1 into master Aug 3, 2018
@ning-y ning-y deleted the pretty-mcq branch August 3, 2018 11:47
@remo5000 remo5000 mentioned this pull request Aug 8, 2018
1 task
Aulud pushed a commit to Aulud/cadet-frontend that referenced this pull request May 25, 2020
* Updated tests - /grading should show group

* Augmented database query - now groups are visible

* Updated Swagger Documentation

* Updated tests - changed group to groupName

* Style change to API - group now is groupName

* Swagger Documentation Change for group to groupName
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.

3 participants