Skip to content

Fix display of MCQ/MRQ choices with long text. - #139

Merged
itsjeyd merged 3 commits into
masterfrom
fix-long-choices
Jan 27, 2017
Merged

Fix display of MCQ/MRQ choices with long text.#139
itsjeyd merged 3 commits into
masterfrom
fix-long-choices

Conversation

@itsjeyd

@itsjeyd itsjeyd commented Jan 24, 2017

Copy link
Copy Markdown
Member

Reported by McKinsey.

cf. OC-2264

Screenshots (Apros)

MCQ:

mcq

MCQ during review (with tip):

mcq-review-tip

Rating question:

rating

Rating question during review (without tip):

rating-review

MRQ:

mrq

MRQ during review (without tip):

mrq-review

Test instructions

  1. Add a Step Builder block to a unit, setting "Max. attempts allowed" to a non-zero value, and "Extended Feedback" to True.

  2. Add a Mentoring Step block and a Review Step block to the Step Builder block.

  3. Add an MCQ, Rating, and MRQ to the Mentoring Step block, setting the text of some choices to a long string.

  4. Add tips for individual choices to the MCQ.

  5. Add a Score Summary block to the Review Step block.

  6. Publish the unit and navigate to it in the LMS/Apros. Observe that alignment of choice texts matches screenshots.

  7. Answer the questions, submit, and review grade until you have no attempts left. Use links to individual questions to review answers. Observe that alignment of choice texts matches screenshots.

Reviewers

@bradenmacdonald

Copy link
Copy Markdown
Member

@itsjeyd Looks much better - I'm just wondering; did the MCQ question text used to be full width or have they always been half-width like this?

@itsjeyd

itsjeyd commented Jan 24, 2017

Copy link
Copy Markdown
Member Author

@bradenmacdonald I'm not sure if they've always been half-width, but they have been in the more recent past. This PR does not change that particular aspect of the layout.

@mtyaka mtyaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@itsjeyd It looks very nice, I just found one small issue with paddings. When that's fixed, this is +1.

👍

  • I tested this: I followed the testing instructions and tested various combinations of questions in both problem builder and step builder.
  • I read through the code
  • I checked for accessibility issues


.mentoring .choices-list .choice-label-text {
display: table-cell;
padding-left: 0.4em;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should add some padding to the right as well - currently there's no padding between the choice text and the line that divides the text from the tip:

screen shot 2017-01-27 at 09 51 01

Ideally it would be the same amount of padding on both sides, to make it easier to support RTL.

@mtyaka

mtyaka commented Jan 27, 2017

Copy link
Copy Markdown
Member

@itsjeyd While testing this, I found that the layout is currently somewhat broken in RTL mode, but it's also broken on master, so I'm just including the screenshots here for reference, but fixing this is out of scope of this PR. I don't really remember whether problem-builder was ever RTL compatible.

screen shot 2017-01-27 at 09 53 37

screen shot 2017-01-27 at 09 54 36

@itsjeyd

itsjeyd commented Jan 27, 2017

Copy link
Copy Markdown
Member Author

@mtyaka Thanks for the review, and for catching the padding issue. I was using the Apros theme, which made it difficult/impossible to notice. Fixed in the most recent commit, please have another look.

padding-lms

@mtyaka

mtyaka commented Jan 27, 2017

Copy link
Copy Markdown
Member

👍 thank you

@itsjeyd
itsjeyd merged commit e620ce7 into master Jan 27, 2017
@bradenmacdonald

Copy link
Copy Markdown
Member

@bradenmacdonald I'm not sure if they've always been half-width, but they have been in the more recent past. This PR does not change that particular aspect of the layout.

@itsjeyd As I feared, QA reviewed this and said the half-width MCQ text is not correct.

Form further investigation I think it comes from #125. @pomegranited was this change deliberately made for a11y or is this a side effect?

@itsjeyd Can we add a CSS rule to the Apros CSS to override that?

@pomegranited

Copy link
Copy Markdown
Contributor

@bradenmacdonald CC @itsjeyd IIRC, the choices width was adjusted simply to make tips appear alongside the choices, not as a specific a11y change.

@xitij2000
xitij2000 deleted the fix-long-choices branch December 13, 2019 06:43
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.

4 participants