Presentation updates

[e-kolpakov]

JIRA Ticket: https://openedx.atlassian.net/browse/OSPR-543
Details: See JIRA ticket
Sandbox: LMS, CMS
Merge deadline: Friday 24th (in time for upcoming release)

Testing instructions:

1. Set up a couple of mentoring XBlocks
   2, Set up Dashboard block to draw data from those blocks
2. Set custom value for "Label for average value"
3. Set "Display numeric values" to False
4. Set some "Questions to be hidden" (see help string). Note that only questions are counted toward question index, e.g. if Mentoring contains freeform answer and MCQ, MCQ index would be 1, not 2.
5. Configure visual representation according to help string.
6. Publish
7. Go to LMS, provide answers for mentoring blocks
8. Navigate to dashboard block

Observe the following:

• Visual progress image is horizontally centered.
• Average values have custom label specified at step 3
• Numeric values are not displayed, only color codes are available
• Questions configured to be excluded are excluded and not participate in average calculations.

[sarina]

You ought to add in some validation here, in case someone puts in data in an unreadable format (for example swaps [] for the { } or something).

[sarina]

eg maybe,

try:
    hide_questions = self.exclude_questions.get(mentoring_block.url_name, [])
except Exception:
    # log exception & what self.exclude_questions is

[bradenmacdonald]

+1

[sarina]

I see you have two travis builds running on this repo (you have both the push job and the pr job enabled). You can pretty easily fix this:

We noticed within the last couple of days that some Travis-run repos are starting two jobs for a pull request. (One job labeled "continuous-integration/travis-ci/pr" and another labeled "continuous-integration/travis-ci/push").

If this is happening to you, you will need the following stanza in your .travis.yml:

branches:
    only:
      - master

Example fix here

[sarina]

@e-kolpakov this seems reasonable. Do you have a sandbox where I can look at this?

[bradenmacdonald]

@sarina We are actually deliberately running two Travis builds on this repo. One build is just for this branch (the push build), and one tests the result of the merge (the pr build). In addition, we found that the "fix" you mention above seems to sometimes prevent builds when PRs are opened against non-master branches.

Edit: another advantage of leaving the "push" build enabled for other branches is that you can get build results before you open a PR, and then when you do open the PR, it will show any intermediate commit builds that it ran during development.

[sarina]

@bradenmacdonald ok, as long as you're fine with it. It was annoying on my repos, but I'm not doing lots of fancy stuff and didn't want to annoy Travis.

[bradenmacdonald]

Put a period and space after the ) here. It is currently appearing as "has number 1)Examples: "

[bradenmacdonald]

Also, I'd suggest you rephrase: "Optional rules to exclude specific questions both from displaying in dashboard and from the calculated average. Rules must start with the url_name of a mentoring block, followed by list of question numbers to exclude (the first question being number 1). Must be in JSON format. Examples: ..."

[bradenmacdonald]

@e-kolpakov I forgot to mention this earlier, but the current way that one sets "Questions to be hidden" using JSON is not very user-friendly.

If you have the time and are feeling ambitious: you could modify this XBlock so that its author_view is almost the same as the normal student_view but author_view includes a [x] Hide checkbox next to each question row. You could also make the "Average" row labels into text inputs in the author_view. The only tricky thing is you need to use new JavaScript to call a handler method whenever it wants to save changes. I'm not sure if that approach goes against Studio UX design goals, but it sure would be easier to use! (I've been wanting to do something similar for choices in Problem Builder, but haven't had the chance yet)

[e-kolpakov]

@bradenmacdonald Making "Questions to be hidden" is a good suggestion, but this task is due tomorrow and I'm not feeling ambitious :) So please add this to backlog as an improvement.

[e-kolpakov]

@sarina sandbox: not yet, I'll spin it up when I have time (probably in a matter of one hour from now)

UPD: sandbox provisioned, link added to PR description

[e-kolpakov]

@sarina just a reminder, it would be great if this could be merged today, in time for upcoming release.

[sarina]

Sure thing. Is there a sandbox available to take a look at it?

On Fri, Apr 24, 2015 at 3:27 AM, Eugeny Kolpakov notifications@github.com
wrote:

@sarina https://github.com/sarina just a reminder, it would be great if
this could be merged today, in time for upcoming release.

—
Reply to this email directly or view it on GitHub
#16 (comment)
.

[antoviaque]

@sarina Yup, you have the links in the PR description above - it took me some time to find them too : )

[sarina]

Typo: "dissplayed"

[sarina]

@e-kolpakov 👍 to merge once the typo is addressed.

I do want @cptvitamin to take a look at this block, even if it's after it's merged. He will be back from vacation next week. Could you please keep the sandboxes up until he's gotten a chance to review?

[e-kolpakov]

@sarina there's one feature left not implemented: the ability to set up average labels per mentoring (now it's one setting for dashboard). Since this already get thumbs up and release is quite close, I'll merge this PR when the build turns green, and implement that in a follow up PR (probably today, Monday for sure).

[sarina]

@e-kolpakov you're going to keep the sandbox running yes?

[e-kolpakov]

@sarina yes, once it's not needed it is just freed to be used later, but not shutdown/disabled/etc.

[cptvitamin]

@e-kolpakov I've never seen anyone try to use aria-label on a td element, and I don't blame you for trying since the spec says it is allowed. However, I wanted to perform a quick implementation test for using aria-label on a td element and I found that only Voiceover will read the aria-label, and only if there is an explicit value in the td element, and it reads both. If the cell is empty, it ignores the aria-label. Testing in NVDA and JAWS revealed that only explicit td values are read. aria-labels are ignored.

I recommend using off-screen text (class=sr) inside the table cells in this use case.