Add "Edit" button to each leaf xblock on the container page

[andy-armstrong]

This is the set of changes to implement STUD-1306. The current xblock edit modal on the unit page has been replaced with a newly designed modal. The new one is a true modal that appears in the middle of the window, and it can be opened from any page (the existing one only works if an xblock has been rendered directly onto the page). The primary reason for the new modal is to support editing from the container page, where components can be nested and are not rendered as individual xblocks.

Note: the old modal has been replaced on the unit and "Pages" page too.

You can view these changes on my sandbox using "Test Course":

http://studio.andy-armstrong.m.sandbox.edx.org/settings/advanced/AndyA.AA101.AndyA/branch/draft/block/AndyA

[andy-armstrong]

@mhoeber This story should be ready for doc now. Let me know if you have any issues with it.

[andy-armstrong]

@mhoeber I've fixed a lot of issues including (I hope) all the ones you reported. I've pushed again to the sandbox so please try this again and let me know how it looks.

[mhoeber]

@andy-armstrong Looks like the public/private issues are good. Two issues:

• In a child problem component, the editor doesn't show the link for the advanced editor, so you can't edit the problem. May be a problem with the new editor integration, but for a problem on the unit page, you do see the advanced editor link. See course I made on your sandbox, A/B test child for example
• Don't forget the text string in the child page publishing status: "Say something useful about . . ."

I believe ongoing unpublished doc at: http://draft-nested-components.readthedocs.org/en/latest/building_course/organizing_course.html#nested-components is good for being in synch with this story.

This is in PR 2751

[cahrens]

Do you really want this to say "xblocks" as opposed to something more generic like "components"?

[andy-armstrong]

I just took the JIRA title as I thought that's what we'd settled on. It's funny because in some ways xblocks are more generic, in that not all xblocks are components. However, from a non-technical standpoint, component is the more generic term. In the code there really is no notion of components, so all my new classes are called xblocks. From a user standpoint, though, the name xblock should never be seen.

[andy-armstrong]

@mhoeber I've updated the sandbox again so let us know if you see any issues. I've also uploaded a test project that you can use:

http://studio.andy-armstrong.m.sandbox.edx.org/course/AndyA.AA101.AndyA/branch/draft/block/AndyA

[nasthagiri]

Aren't there 3 possible values for publish_state?

[frrrances]

Yes, but we are downplaying the options since they don't really matter. There are really two meaningful ones: (1) states that allow editing (unpublished/private and draft), and (2) the state that doesn't allow editing (published/public).

[andy-armstrong]

Yes, but @frrrances is deprecating the use of 'private' in the UX. So the code only distinguishes between published (read-only) and any other state (editable).

[auraz]

One more issue. Same page, two videos:

This is after page reloads.

[cahrens]

I'd still like to see this mock file simplified to remove extra cruft so that Nimisha, you and I can all use it for our tests.

Maybe the one in @nasthagiri's PR would work? https://github.com/edx/edx-platform/pull/3234/files#diff-86

[andy-armstrong]

Agreed. I'll take @nasthagiri's file and rework the tests to accommodate it.

[nasthagiri]

Let me know if you need me to make any changes to the mock file to
accommodate your tests.

On Wed, Apr 9, 2014 at 11:23 AM, Andy Armstrong notifications@github.comwrote:

In cms/templates/js/mock/mock-container-xblock.underscore:

•                                            <i class="icon-edit"></i>
  

•                                            <span class="action-button-text">Edit</span>
  

•                                        </a>
  

•                                    </li>
  

•                                </ul>
  

•                            </div>
  

•                        </header>
  

•                        <article class="xblock-render">
  

•                            <div class="xblock xblock-student_view xmodule_display xmodule_VideoModule xblock-initialized" data-runtime-class="PreviewRuntime" data-init="XBlockToXModuleShim" data-runtime-version="1" data-usage-id="i4x:;_;_AndyA;_ABT101;_video;_fbd800d0bdbd4cb69ac70c47c9f699e1" data-type="Video" data-block-type="video">
  

•                                <h2>Video</h2>
  

•                                <div id="video_i4x-AndyA-ABT101-video-fbd800d0bdbd4cb69ac70c47c9f699e1" class="video closed is-initialized" data-streams="1.00:OEoXaMPEzfM" data-save-state-url="/preview/xblock/i4x:;_;_AndyA;_ABT101;_video;_fbd800d0bdbd4cb69ac70c47c9f699e1/handler/xmodule_handler/save_user_state" data-caption-data-dir="None" data-show-captions="true" data-general-speed="1.0" data-speed="null" data-saved-video-position="0.0" data-start="0.0" data-end="0.0" data-transcript-language="en" data-transcript-languages="{&quot;en&quot;: &quot;English&quot;}" data-autoplay="False" data-yt-test-timeout="1500" data-yt-api-url="www.youtube.com/iframe_api" data-yt-test-url="gdata.youtube.com/feeds/api/videos/" data-transcript-translation-url="/preview/xblock/i4x:;_;_AndyA;_ABT101;_v
  

  ideo;fbd800d0bdbd4cb69ac70c47c9f699e1/handler/transcript/translation" data-transcript-available-translations-url="/preview/xblock/i4x:;;_AndyA;_ABT101;_video;_fbd800d0bdbd4cb69ac70c47c9f699e1/handler/transcript/available_translations" data-autohide-html5="False" tabindex="-1">

•                                    <div class="focus_grabber first" tabindex="-1"></div>
  

Agreed. I'll take @nasthagiri https://github.com/nasthagiri's file and
rework the tests to accommodate it.

Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/2981/files#r11440942
.

edX | nasthagiri@edx.org andya@edx.org

141 Portland Street, 9th floor

Cambridge, MA 02139
http://www.edx.org http://www.edxonline.org/

[image:
http://www.e-learn.nl/media/blogs/e-learn/edX_Logo_Col_RGB_FINAL.jpg?mtime=1336074566]

[andy-armstrong]

@auraz That is a very strange screenshot, as that is showing both the old and the new styling for the modal. @frrrances do you have any ideas? I'll see if I can reproduce it.

[auraz]

@andy-armstrong Steps to reproduce for both issues (color of header and upload modal positioning): create new video player without save and page reload.

[cahrens]

The CodeMirror problem editor is not sizing correctly within the new modal (reproducible for me on both Firefox and Chrome). There is blank space at the bottom, and scrolling does not behave correctly.

[andy-armstrong]

@cahrens The sizing bug is due to a change we just put in for the LaTex editor. @frrrances, I guess this needs to be scoped more tightly to the LaTex editor.

[andy-armstrong]

@auraz Thanks, I am able to reproduce it following those steps. @frrrances and I will investigate why.

[andy-armstrong]

@auraz I've fixed the problem with editing the new components. I'd added logic to add a new data attribute (data-category) when rendering existing items, but didn't add the corresponding code for items newly added to the page. I've changed the code to no longer require that new data attribute and to instead find the category elsewhere.

[andy-armstrong]

@cahrens @nasthagiri I've addressed your next round of comments, fixed a couple of bugs, and improved the Python container test to verify drafts as well as published containers. Sorry to keep asking this, but... can you re-review?

[cahrens]

I looked over the last commit (did not check out and test again). I think the only remaining issue is the LaTeX High Level Source problem (note that I do not see the Edit button problem on Firefox that Nimisha sees).

Once LaTeX is fixed, 👍

[cahrens]

@frrrances --
@nasthagiri is going to a meeting but wanted to log an issue that she saw. When the window is narrow, the modal dialog header wraps oddly:

[marcotuts]

@frrrances A couple notes about the pages I've started looking at.
1.) It seems the edit button (on components) has more space off to its right than the display name has to its left, this may be a percentage sizing for the flex element or a margin-left: X% missing from the component-actions area?
2.) On the container page the expand/collapse buttons are kept in focus after clicking them, and their right/left outline is shown. We may want to hide that outline in its entirety?
3.) Similar to number #1, inside of a component for the header bar it looks like there is more space to the right of the BASIC/ADVANCED, EDITOR/SETTINGS buttons than there is spacing on the left for the display name.
4.) I'm seeing something slightly different than @auraz (I'm in Firefox) but related to the upload transcript upload flow - https://www.dropbox.com/s/r11a76cu11ahnst/Screenshot%202014-04-09%2010.52.25.png
5.) How easy/hard would it be to make the edit HTML modal be either larger than the component window to cover it? I tinkered with percentages for the left, top, and width sizing to make it appear initially larger than the component, (though scrolling would still show you the other modal underneath.)
6.) The container page has a gray border around the entire page that I don't think needs to be there (or needs border-radius adjusted to fit properly if we'd like to keep it- https://www.dropbox.com/s/hy7m5x0z31ovjxt/Screenshot%202014-04-09%2013.25.00.png
7.) Probably not included in this PR, but it would be nice to clean up text alignment and coloring for active/not-active tabs in the image upload (within HTML editor) https://www.dropbox.com/s/sod0kdoqp4oujvb/Screenshot%202014-04-09%2013.25.59.png
8.) Visual comment/feedback - The drop-shadow on the main active white area for the modals felt a bit heavy to me considering the adjacent drop-shadow and gray page background, not critical either way.
9.) Double checking that Course Updates and Course Handouts are expected to use the old modal pattern still. I presume so, but I just wanted to check.

I'll be looking at FED right after this!

[marcotuts]

is this specifically to force the modals to pages where we've added body.view- tags? In the case where we've added these tags to all studio pages, would it make sense to specifically identify the views that use it vs. the views that don't? No revision suggested here, just wanted to understand the role of the specificity.

[ghost]

it is a replacement for the body tag, scoping the modal actions to the view/body. it is used sometimes to add classes to the body (like "&.modal-window-is-shown") to show and hide the modals. (you can see this behavior in the outline sass. for the container page, i ended up using js to open and size the modal so we can make it responsive - or at least to the extent that we can now.)

[ghost]

@marcotuts Thanks for the review! Here are notes on each item of your list:

1. percentage widths on the header title and the action list: the extra lets them not wrap when the window shrinks. we can move these away from flex-box/display inline but i think this is preferable... let me know if you think otherwise.
2. focus state removed on the group expand collapse
3. same as Put back in the blocks for Advanced Settings. #1
4. any idea how you got in that state with the upload translations window? i can't replicate...
5. the html modal is a plug in for tinymce. i think this should be grouped with Video tests fix #7
6. good catch. border around whole container removed.
7. yeah the image upload is a plug in for tinymce - not part of this PR, but good to note
8. the modal content area looked really washed out and fuzzy when the box-shadow was lighter, and too flat without any shadow. let me know if you have suggestions!
9. yes, the course updates and handouts are code mirror - they will eventually be updated with tinymce. someday. when the new tinymce finally gets released. the textbooks pdf upload modal and the section release date modal on the outline page have been updated though.

[marcotuts]

@strikeseason - From the list above everything makes sense and I don't think warrants any additional changes.

For #4, I was testing on Andy's sandbox around the time of my comment ~130pm, using Firefox, but otherwise I'm not sure what it was. Others experienced cache issues earlier in the PR so perhaps it was that, and it worth ignoring if nobody else can reproduce it.

For #8, my only potential suggestion would be whether or not a lighter inset shadow would work, but I think overall this is a huge improvement and any potential shadow tweaks aren't worth slowing down to fix for now.

One last note, I realize I didn't test LaTeX but it seems like others have tested it, so hopefully that's ok.

Awesome work everyone!

[marcotuts]

Aside from one small question about whether or not the wrapper-mast styling to hide the container level edit icons is necessary,everything else has been addressed. In fact, even without this potential change I'm a 👍 !

[nasthagiri]

Sorry guys. I'm still figuring out what is expected to be working and what's not at this point with AB Testing. Is the "View Live" button on Container pages supposed to be functional at this point? I get the following stack trace (in split_test_module.py) when running locally. I am also unsuccessful on Staging (although don't have a stack trace), so it may not be introduced by this PR.

Request Method: GET
Request URL: http://localhost:8000/courses/edx/T101/2014/courseware/ce6b3c3c9d644854837f01f68752bb0d/54471004037c4af4952590ec5337b048/
Django Version: 1.4.8
Exception Type: AttributeError
Exception Value:
'NoneType' object has no attribute 'get_content_titles'
Exception Location: /edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/split_test_module.py in get_content_titles, line 107
Python Executable: /edx/app/edxapp/venvs/edxapp/bin/python
Python Version: 2.7.3

[andy-armstrong]

@nasthagiri You should be able to work with a split_test module that was created in XML, but you can't create one in Studio. I upgraded my sandbox and loaded an XML course and am able to work with a split_test module:

http://studio.andy-armstrong.m.sandbox.edx.org/unit/AndyA.AA101.AndyA/branch/draft/block/split_test_unit

Can you see if you can reproduce the problem you saw on my sandbox? Thanks.

[auraz]

@andy-armstrong thanks for explanation.

[nasthagiri]

@andy-armstrong I am trying to view the a-b test module in LMS. But the problem exists outside of this PR. The rest looks great!
@frrrances Thanks for fixing the edit modal header.

👍 Great work guys!