Image Modal CMS HTML Block subtype #2362

caesar2164 · 2014-01-29T22:38:23Z

Added YAML file for the HTML template code for the modal to work
Added CSS and JS code for modal to look and function properly
Updated code to take comments into account.
Simplified HTML template and expanded JS to set up image modal on load.
Added preliminary drag script.
Converted jQuery UI draggable to Draggabilly

@frrrances - here's the image modal branch! with Draggabilly and everything!

Here are some screenshots of how imageModal works. (This is using the Stanford theme, but would work equally well with the default courseware theme.)

When you first load an image modal it presents like this:

When you click on the image, it goes into the modal interface (starting in "fit to screen"):

Clicking the + magnifying glass makes the image full size and draggable (via draggabilly):

You can click the - magnifying glass to go back to the "fit to screen", or click the X to exit the modal.

cahrens · 2014-01-30T17:46:08Z

Studio team has a story for reviewing this PR (STUD-1256). I recommended to Giulio that he start writing a Jasmine test for imageModal.js. Giulio will also provide documentation on how the feature should work.

@mhoeber Adding you so you are aware of this PR.

cahrens · 2014-01-31T18:32:45Z

At today's planning, Studio thought it made more sense for LMS to review this PR (so @singingwolfboy will not be reviewing). I have moved the Issue, and it is now LMS-2146.

frrrances · 2014-02-01T03:38:06Z

@caesar2164 I haven't had a chance to check out the new draggabilly version yet, but I wanted to check with you about the other image zoom that was merged to master: will that cover this use case as well? It seems like having two image zoom components might be confusing for course authors. If the other one won't serve, can you outline the reasons why it won't work for your use cases? (I'll be out for a few days next week, but will review this as soon as I'm back.)

caesar2164 · 2014-02-01T23:59:27Z

@frrrances - unfortunately, the zooming image won't work for our use case. Also, I think mine is a better starting point for a general "breakout" system to allow users to use their full browser window area, while the zooming image has very limited uses.

Here's a quote from @gbruhns (Stanford courseops) explaining why we need image modal, and can't use "zooming image":

I have a test course with the VPUE Adventures in Writing course content to show why the new "zooming image" HTML component won't work for their course and why it would be good to have Giulio's image modal option. Just to refresh everyone on the VPUE course, they have created animated story boards to present their content. These are large files that get text heavy and shrinking the image down to fit in the courseware pane makes the text hard to read on certain images.

The zooming image component gives the user a magnifying glass and you move your cursor over the image to see that part of the image magnified. I have included a screenshot below. I can see how this would work for certain images, but in the case of a story board where the entire image tells the story, what you really want (and what the VPUE asked for and what Giulio provided) was a way for the image to fill the entire screen and not be bound to the small courseware pane.

Greg

dianakhuang · 2014-02-03T22:25:50Z

common/lib/xmodule/xmodule/js/src/html/imageModal.js

+    var currentDraggie = $(this).siblings('.imageModal-imgWrapper').children('img').removeClass('zoomed').attr('id').split('-');
+    draggie[currentDraggie[1]].disable();
+    $('html').css({overflow: 'auto'});
+  });


I'm not sure how I feel about some of this duplication. Is there any way to split out this logic to a common function where we just pass in a single jQuery object (this or this.closest('.imageModal'), presumably) and do all of the operations on that object?

dianakhuang · 2014-02-04T14:57:50Z

@frrrances is out, so @talbs volunteered to help step in and take a look.

dianakhuang · 2014-02-04T15:07:57Z

common/lib/xmodule/xmodule/js/src/html/imageModal.js

+    // if contents of zoomable link is image and large image link exists: setup modal
+    if (smallImageObject.is('img') && largeImageSRC) {
+      var smallImageHTML = $(this).html();
+      var largeImageHTML = '<img alt="" src="' + largeImageSRC + '" />';


Why is the alt being deliberately left blank here? Why doesn't use the same alt-text as the small image?

talbs · 2014-02-06T15:37:19Z

@caesar2164, I noticed you've made some significant changes based on feedback. Thanks for making that effort! Let me know when you're ready for another look through (and it looks like you have some tests failing currently).

caesar2164 · 2014-02-06T17:54:03Z

@talbs - Phew, it's not my branch that failed, it;s testing that failed in this case. I'll force Jenkins to try again.

Also, yes please give it another look, I think I've addressed everything.

talbs · 2014-02-06T17:57:48Z

cms/templates/base.html

@@ -235,6 +235,10 @@
    <script id="system-feedback-tpl" type="text/template">
      <%static:include path="js/system-feedback.underscore" />
    </script>
+
+    <script type="text/template" id="image-modal-tpl">


Should this be loaded into every Studio page or just the unit editing page where it is needed to reproduce the interaction/UI of the feature when placed in LMS context? @singingwolfboy, not sure if you have a preference on this as well.

The template needs to be present on every page where it might be used, but I don't think that this template can ever be used on any page on Studio except for the unit editing page. Maybe move it to cms/templates/unit.html?

I can try unit.html, although I'll have to add the "header_extras" block

this is now done.

talbs · 2014-02-06T18:04:15Z

@caesar2164, thanks for the ping via HipChat to review the changes you made.

I'm having a very hard time following:

what changes/recommendations/questions you've addressed from my initial review
what resolution (agree, disagree, etc.) you decided to choose for each item addressed
what specific implementation change you made in the cases where you did revise something

Can you please provide comments to my original feedback that supply that info noted above? Once I have that context, I'll be better able to re-review and help support this work.

Thanks!

singingwolfboy · 2014-02-06T18:06:27Z

common/lib/xmodule/xmodule/js/src/html/imageModal.js

+    if (smallImageObject.is('img') && largeImageSRC) {
+      var smallImageHTML = $(this).html();
+      var largeImageALT = smallImageObject.attr('alt');
+      var largeImageHTML = '<img alt="' + largeImageALT + ', ' + gettext("Large") + '" src="' + largeImageSRC + '" />';


Can you move this HTML into the template, as well? And ideally, the smallImageHTML as well?

How would I move the smallerImageHTML into the template? I'm currently using the the HTML straight from the Studio input field.

As I explained, I use the HTML from Studio for the small image.

caesar2164 · 2014-02-06T19:42:55Z

@singingwolfboy & @talbs - code all updated, running tests and comments all addressed with replies.

Could you guys give it another look?

talbs · 2014-02-06T19:44:52Z

cms/templates/unit.html

 <%namespace name="units" file="widgets/units.html" />
 <%block name="title">${_("Individual Unit")}</%block>
 <%block name="bodyclass">is-signedin course unit view-unit</%block>

+<%block name="header_extras">


Doing this will load the script into the head of the HTML structure, correct? For optimization's sake, should this be called right before the </body> to help with page loading and asset gathering?

The template will load wherever the <%block name="header_extras"> is located in <head>...

Thanks. My original concern stands - if this script is not necessary to support the page loading, it should be placed before the close of the body element (which will allow it to not be a dependency that will slow down the loading of the DOM and basic page functions).

This script IS necessary for page load IF there is an image-modal in the unit that is being loaded.

Allow me to clarify further, is this script necessary for the base state (layout, content, etc.) of the page to load?

If not, then I would call it from later in the DOM (i.e. right before the body element closes). See http://www.feedthebot.com/pagespeed/prioritize-visible-content.html for a simple explanation of what JavaScript calls do within the head to loading the basic HTML of the page requested.

Done, moved script to <js_extra>

singingwolfboy · 2014-02-06T19:47:06Z

Looks good to me! 👍

talbs · 2014-02-06T19:47:30Z

@caesar2164 did you see my comment above about explaining your revisions with respect to my feedback (which GH squashed to outdated diffs)? I'll review once more when I can follow your decisions.

caesar2164 · 2014-02-06T21:11:36Z

@talbs I commented on each of your previous comments

dianakhuang · 2014-02-06T21:47:46Z

👍

caesar2164 · 2014-02-06T21:49:16Z

Thanks @dianakhuang!

dianakhuang · 2014-02-06T21:50:14Z

I definitely want @talbs to sign off before merge, though.

caesar2164 · 2014-02-06T21:51:45Z

@dianakhuang, of course! 😄

talbs · 2014-02-06T22:04:28Z

common/lib/xmodule/xmodule/css/html/display.scss

+    width: 95%;
+    margin: auto;
+    overflow: hidden;
+    top: 2.5%;


The ordering here is still incorrect - please place any top, left, bottom, right rules next to the position rule for easier comprehension

caesar2164 · 2014-02-07T00:40:17Z

@talbs, that should be all your comments addressed now. Let me know.

talbs · 2014-02-07T12:21:41Z

Thanks for addressing all of my concerns, @caesar2164. The extra effort is much appreciated.

👍

- Added YAML file for the HTML template code for the modal to work - Added CSS and JS code for modal to look and function properly - Updated code to take comments into account. - Simplified HTML template and expanded JS to set up image modal on load. - Added preliminary drag script. - Converted jQuery UI draggable to Draggabilly

Image Modal CMS HTML Block subtype

* Correctly format date block dates. (cherry picked from commit 22cb307) * ECOM-3673 Update the course end date description text to remove the reference to certificate when course enrollment is in audit mode (cherry picked from commit 649e8b1) * Fixes for courseware date formatting/translation. (cherry picked from commit 80bfa36) Conflicts: lms/djangoapps/courseware/date_summary.py * Fix date summaries with Unicode format issues. This issue would show up when date formatting strings are translated with Unicode characters (like when testing with fake Esperanto translations). (cherry picked from commit c1a59cc) * Converts the dates on the dashboard, sidebar navigation, and important course dates to user specified time zone. (cherry picked from commit 0bf8fc4) Conflicts: common/djangoapps/util/date_utils.py common/lib/xmodule/xmodule/course_module.py common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py lms/djangoapps/ccx/tests/test_models.py lms/djangoapps/courseware/views/index.py lms/djangoapps/student_account/views.py lms/templates/dashboard/_dashboard_course_listing.html openedx/core/djangoapps/content/course_overviews/models.py openedx/core/djangoapps/user_api/preferences/api.py openedx/core/djangoapps/user_api/preferences/tests/test_views.py * Display JST for survey's answer datetime. openedx#2146 * Display JST for section's deadline and course start/end datetime. openedx#2146 * Display JST for face-to-face start/end datetime on Django Admin. openedx#2146 * Display JST for ORA2 settings. openedx#2146 * Add the validation of the schedule dates before 1900 openedx#2284 (openedx#2298) * Implements command count_initial_registration_course openedx#2221 (openedx#2296) * Add GaCourseEditorRole and GaLmsCourseStaffRole openedx#2150 (openedx#2306) * Add GaCourseEditorRole and GaLmsCourseStaffRole openedx#2150 * Add UT for studio openedx#2150 * Fix GaCourseEditor has access to after the course closed openedx#2150 * GaLmsCourseStaff can use the same function as the course staff at the staff debug info openedx#2150 * Fix UT and Bok choy openedx#2150 * Remove extra line feed openedx#2150 * Fix comment openedx#2150 * Mod conditions openedx#2150 * Move judgment conditions to template openedx#2150 * Fix comment openedx#2150 * Remove unused code openedx#2150 * Mod conditions for GaCourseEditor but a global staff openedx#2150 * Revert parameter CourseDetails.update_from_json openedx#2150 * Mod check for staff acccess at LMS for GaCourseEditor openedx#2150 * Rename GaCourseEditorRole -> GaGlobalCourseCreatorRole openedx#2150 * Rename GA_COURSE_EDITOR_USER_INFO -> GA_GLOBAL_COURSE_CREATOR_USER_INFO openedx#2150 * Rename GA_ACCESS_CHECK_TYPE_COURSE_EDITOR -> GA_ACCESS_CHECK_TYPE_GLOBAL_COURSE_CREATOR openedx#2150 * Rename Ga_Course_Editor -> Ga_Global_Course_Creator openedx#2150 * Rename gacourseeditor -> gaglobalcoursecreator openedx#2150 * Rename GaCourseEditor -> GaGlobalCourseCreator openedx#2150 * Rename course_editor -> global_course_creator openedx#2150 * Mod hide staff debug info for GaGlobalCourseCreatorRole openedx#2150 * Fix UT for GaGlobalCourseCreator openedx#2150 * Rename UT openedx#2150 * Fix skipped UT openedx#2150 * Remove unused code openedx#2150 * Fix argument openedx#2150 * Remove unused code openedx#2150 * Fix ambiguous conditions openedx#2150 * Rename GaLmsCourseStaffRole -> GaCourseScorerRole openedx#2150 * Rename GA_LMS_COURSE_STAFF_USER_INFO -> GA_COURSE_SCORER_USER_INFO openedx#2150 * Ga_Lms_Course_Staff -> Ga_Course_Scorer openedx#2150 * Rename galmscoursestaff -> gacoursescorer openedx#2150 * Rename GaLmsCourseStaff -> GaCourseScorer openedx#2150 * Rename lms_course_staff -> course_scorer openedx#2150 * Fix translation openedx#2150 * Fix comment openedx#2150 * Add personal info mask at resign openedx#2148 (openedx#2316) * Add personal info mask at unenroll openedx#2148 * Fix Review. * Add Unit Test. * Add JST date fields near UTC date fields. openedx#2230 (openedx#2307) * Add JST date fields near UTC date fields. openedx#2230 * Fix Review * Fix Review 2. * Add yml Files * Add option of progress restriction that makes users need to pass prob… (openedx#2305) * Add option of progress restriction that makes users need to pass problems. openedx#2147 * Skip Bok-Choy CoursewareProgressRestrictionTest temporarily because this may cause an error of other test * Fix review 1 openedx#2147 * Fix review 2 openedx#2147 * Fix to use different username and email from test_ga_register.py so that no error occurs on test_register_and_activate. openedx#2147 * Fix review 3 openedx#2147 * Fix review 4 openedx#2147 * Fix review 5-1 * Fix review 5-2 * Fix review 5-3 * Fix review 5-4 * Fix review 5-5 * Fix review 5-6 * Fix review 5-7 * Fix review 5-8 * Fix review 5-9 * Fix review 5-10 * Fix review 5-12 * Fix review 5-14 revert 5-13 * Fix review 5-15 * Fix review 5-16 * Fix review 6-1 openedx#2147 * Fix review 6-2 openedx#2147 * Fix review 6-3 openedx#2147 * Fix UTC message in survey page. openedx#2340 (openedx#2345) * Fix bug for manage_user_standing page. openedx#2347 (openedx#2348) * Fix bug and add failed testcase for mask_resigned_user command. openedx#2362 (openedx#2365) * Add confirm for account_disabled of manage_user_standing openedx#2366 (openedx#2367)

ghost assigned adampalay Jan 29, 2014

dianakhuang reviewed Feb 3, 2014
View reviewed changes

dianakhuang reviewed Feb 4, 2014
View reviewed changes

talbs reviewed Feb 6, 2014
View reviewed changes

singingwolfboy reviewed Feb 6, 2014
View reviewed changes

talbs reviewed Feb 6, 2014
View reviewed changes

caesar2164 added a commit that referenced this pull request Feb 7, 2014

Merge pull request #2362 from edx/giulio/image-modal-squash

7320513

Image Modal CMS HTML Block subtype

caesar2164 merged commit 7320513 into master Feb 7, 2014

caesar2164 deleted the giulio/image-modal-squash branch February 7, 2014 17:41

Image Modal CMS HTML Block subtype #2362

Image Modal CMS HTML Block subtype #2362

Conversation

caesar2164 commented Jan 29, 2014

cahrens commented Jan 30, 2014

cahrens commented Jan 31, 2014

frrrances commented Feb 1, 2014

caesar2164 commented Feb 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianakhuang commented Feb 4, 2014

Choose a reason for hiding this comment

talbs commented Feb 6, 2014

caesar2164 commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talbs commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar2164 commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singingwolfboy commented Feb 6, 2014

talbs commented Feb 6, 2014

caesar2164 commented Feb 6, 2014

dianakhuang commented Feb 6, 2014

caesar2164 commented Feb 6, 2014

dianakhuang commented Feb 6, 2014

caesar2164 commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar2164 commented Feb 7, 2014

talbs commented Feb 7, 2014