Changes to the FeedbackXBlock (formerly RateXBlock)#1
Changes to the FeedbackXBlock (formerly RateXBlock)#1pmitros wants to merge 45 commits intopmitros/edx-prototype-releasefrom
Conversation
Starting aggregate views
pmitros
force-pushed
the
master
branch
4 times, most recently
from
1628680 to
3642dc8
Compare
README.md
Outdated
There was a problem hiding this comment.
Put feedback in double quotes ("feedback") to help reduce the number of people who attempt to enter strings in Advanced Setting with single quotes (which doesn't work because it is not valid JSON).
|
I think there is a pretty serious problem with using keyboard navigation to select the rating. As you use the arrow keys to move between the different ratings, it seems to vote on each one. This will cause confusion both for anyone trying to understand the data, and the user since one expects to be able to navigate without actually submitting a rating. FYI @cptvitamin |
There was a problem hiding this comment.
For accessibility, you have to group the radio buttons with the same name together. See
https://dequeuniversity.com/rules/axe/1.1/radiogroup
FYI @cptvitamin
There was a problem hiding this comment.
This appears to be OK. I think it is being used in here:
https://github.com/pmitros/FeedbackXBlock/pull/1/files#diff-4f75d70cd74efb92c6b338163f497b8fR5
So it is contained in a group.
|
re: Save not having visual indication when it has focus and related issues in that comment: Those are Studio-wide issues, actually. |
|
@cahrens @cptvitamin I think this is good to go? |
| <label class="label setting-label" for="freeform">Freeform prompt</label> | ||
| <input class="input setting-input" name="freeform" id="freeform" value="{freeform}" type="text" /> | ||
| <label class="label setting-label">Freeform prompt | ||
| <input class="input setting-input" name="freeform" id="freeform" value="{freeform}" type="text" /> |
There was a problem hiding this comment.
Nit: The thing that's nice about doing the nesting is that you then do not need the IDs (id="freeform"), assuming they aren't being used elsewhere. And in general, we avoid IDs.
|
👍 on my comments (though you could most likely delete some more IDs if so inclined) |
|
@cahrens Thanks. I kept the |
| </fieldset> | ||
|
|
||
| <div class="feedback_header_div"> | ||
| <label class="feedback_header" for="feedback_freeform_textarea">{freeform_prompt}</label> |
There was a problem hiding this comment.
@pmitros this label does not programmatically link to the textarea below. the for attribute must take an IDref. This references the name attribute. Recommend adding an IDref to the textarea below. You may need to append a random value to the end of the ID since multiple Feedback blocks on one page would cause uniqueness errors that would actually impact accessibility (not all do).
|
@pmitros I noticed that the live region feedback "thanks for voting" appears before the actual submit button. live regions are still in the dom and accessible in the order they are encountered. I found that after exiting the textarea, I was informed "Thanks for voting" leaving me to believe my job was done, my vote has been counted. Yet, I didn't hit the submit feedback button yet. I would recommend either removing this notification after a timeout, or placing it after the submit button or, having it appear AFTER the submit button is pressed, not a radio-button selected. also, the Thanks for voting notification is announced after moving from one radio button to the next. If you decide to choose radio button option 5 you have to hear it 5 times. |
|
|
||
| <li class="field comp-setting-entry is-set"> | ||
| <div class="wrapper-comp-setting"> | ||
| <label aria-describedby="lik_fair" class="label setting-label">Likert option 4</label> |
There was a problem hiding this comment.
two closing label elements here (see line 86)
|
@pmitros while I'm glad you converted your studio markup from explicit to implied labelling, aside from the duplicate closing
The implied labels are constraining the overall width of the form controls. That is how I noticed the duplicate closing elements. This concerns me because we have to start fixing the labels across the studio product, either converting to implied or adding explicit labels. It appears taht both solutions will require significant effort. |
Initial support for GitHub actions
support for python 3.5/3.8 and django 2.2 + basic tests
it was not versioned
bump to v1.1
feat: add support for django 3.2
* Add python condition for installing bok-choy * Remove unsupported python 2.7 * Add constraints to requirements given python version
This change removes support for Python 2, in order to fix the github actions build. If you would like to continue to use older versions of Open edX (e.g. to replay old courses), please use an earlier commit of this tool as well.
The problem is cgi.escape() was deprecated since py3.2 and removed since py3.8
fix: edit mode error. cgi library obsolete, replaced with HTML
fix: relevant documentation link
6 participants
No description provided.