Unification of forks & conversion to real children - #4
Conversation
…efresh-fixes2 Design refresh fixes2
Use StringIO instead of cStringIO to support Unicode in LightChildren
When user submits the answers, the results (checkmarks/popups/tips/messages) are displayed, and the submit button is disabled. If user then changes any of the answers, the results are cleared and the submit button re-enabled (but only if the answers pass validation).
…flow Tip overflow
Clear results when answer is changed.
It used to close when any part of the popup would be clicked.
…ick-close Only close the feedback popup when clicking on the close 'x' button
Style changes
* Proper alignment when MRQ/MCQ choices span over more than one line * Show disabled submit button upon first load * Remove extra color from submit button * Size of feedback title
CSS Fixes - Additional fixes of CSS from QA
Move to use opaque keys
…nforce_type Make use of the new optional enforce_type flag
…lay-name Add `display_name` to the Data export XBlock.
…assessment-mode Mentoring assessment mode
Once a student submits an answer in assessment mode, he should not be allowed to change it, so stop listening for changes once an answer has been submitted. The change lister would re-enable the submit button if the student changed their answer, which shouldn't happen in assessment mode.
Keep submit button disabled after answering in assessment.
It used to be on a separate proeperty - it's now using a real XBlock field and its default value
There was a problem hiding this comment.
I reiterate my concern about using fallback_view, and not knowing which fields will be available.
|
@cpennington Thanks for the review. I have addressed the rest of your notes in d5ea6f6 |
There was a problem hiding this comment.
If self.display_name is set, do you want to return that? Or are you intending to always override it?
There was a problem hiding this comment.
For now we are intending to always override it. You can see that display_name is not in the editable_fields list so it is not settable in Studio.
In a future PR that I'll be porting from another fork of this, we will add support for a settable display_name for these blocks.
|
@cpennington Can this get your thumbs up now, or do you still need to review? |
|
👍 |
|
@bradenmacdonald - now it has two thumbs! What's the right process here? Are you going to open up a PR for the block into edx-platform and close this one? |
|
@sarina I don't know if there's any plan to move this into edx-platform. For now I am just going to close this PR. Since the new code is already on the open-craft/master branch, there's nothing else to do and in the near term we will just work from open-craft/master. I'll let @antoviaque discuss the long-term plan. I think part of it will involve creating a new repo with a new name for this block. |
|
Wait, what? I thought this was for Harvard. If we're not going to put it on edx.org why are we reviewing it? |
|
@sarina Yes, it's definitely going on edx.org, I just don't think it's going into the edx-platform repo specifically. Sorry for any confusion. |
|
OK. I'm going to mark the OSPR review on this "done" because it's got the thumbs up, and leave it to you and Xavier to figure out what the next steps are. |
|
@sarina Great, thanks. |
|
@bradenmacdonald when @sarina said, "open up a PR for the block into edx-platform," she meant a one-line PR to add this repo to requirements/edx/edx-private.txt |
|
Ah, that makes sense :) Sorry for misunderstanding. |
|
Closing this PR now. Thanks for the reviews everyone. Note: Travis status shows an error currently but this exact commit was passing previously (link). There seems to be an issue with Travis today affecting long-running builds (example from another repo). |
|
@sarina @bradenmacdonald Great that this is merged - yes we'll need to update the requirements in edx-platform. Braden do you want to have the honor? : ) The requirement to update is in https://github.com/edx/edx-platform/blob/master/requirements/edx/edx-private.txt , plus the reference to xblock-utils in https://github.com/edx/edx-platform/blob/master/requirements/edx/github.txt . Note that we'll need to bump the requirements hash again once https://openedx.atlassian.net/browse/OSPR-478 is merged too, but it would be good to already get the current version live already. |
12 participants
Note that this PR isn't meant to be merged, it just serves to show the difference with what is on edx.org. Note that the two have slightly diverged, due to backports of features. We would likely switch the requirements used on edx.org to use the current repository's master rather than merging.
See https://openedx.atlassian.net/browse/OSPR-419
Sandbox: LMS: http://sandbox.opencraft.com/ - CMS: http://sandbox.opencraft.com:18010/