Addressed code review issues.

* Fixed lint/pep8 issues in test py
* Implemented itsjeyd's and bradenmacdonald's suggestions

drag_and_drop_v2/public/js/drag_and_drop.js

@@ -79,16 +79,15 @@ function DragNDropTemplates(url_name) {
             style['outline-color'] = item.color;
         }
         if (item.is_placed) {
-            // Allow for the input + button width for aligned items
-            if (item.zone_align) {
+            if (item.zone_align === 'none') {
+                style.left = item.x_percent + "%";
+                style.top = item.y_percent + "%";
+            } else {
+                // Allow for the input + button width for aligned items
                 if (item.input) {
                     style.marginRight = '190px';
                 }
             }
-            else {
-                style.left = item.x_percent + "%";
-                style.top = item.y_percent + "%";
-            }
             if (item.widthPercent) {
                 style.width = item.widthPercent + "%";
                 style.maxWidth = item.widthPercent + "%"; // default maxWidth is ~33%
@@ -156,7 +155,7 @@ function DragNDropTemplates(url_name) {
         // and render its placed items as children
         var item_wrapper = 'div.item-wrapper';
         var items_in_zone = [];
-        if (zone.align) {
+        if (zone.align !== 'none') {
             item_wrapper += '.item-align.item-align-' + zone.align;
             var is_item_in_zone = function(i) { return i.is_placed && (i.zone === zone.title); };
             items_in_zone = $.grep(ctx.items, is_item_in_zone);
@@ -171,7 +170,7 @@ function DragNDropTemplates(url_name) {
                         'dropzone': 'move',
                         'aria-dropeffect': 'move',
                         'data-zone': zone.title,
-                        'data-zone_align': zone.align,
+                        'data-zonealign': zone.align,
                         'role': 'button',
                     },
                     style: {
@@ -247,7 +246,7 @@ function DragNDropTemplates(url_name) {
         var is_item_placed = function(i) { return i.is_placed; };
         var items_placed = $.grep(ctx.items, is_item_placed);
         var items_in_bank = $.grep(ctx.items, is_item_placed, true);
-        var is_item_placed_unaligned = function(i) { return i.is_placed && !i.zone_align; };
+        var is_item_placed_unaligned = function(i) { return i.zone_align === 'none'; };
         var items_placed_unaligned = $.grep(items_placed, is_item_placed_unaligned);
         return (
             h('section.themed-xblock.xblock--drag-and-drop', [
@@ -608,7 +607,7 @@ function DragAndDropBlock(runtime, element, configuration) {
             $anchor = $zone;
         }
         var zone = $zone.data('zone');
-        var zone_align = $zone.data('zone_align');
+        var zone_align = $zone.data('zonealign');
         var $target_img = $root.find('.target-img');
 
         // Calculate the position of the item to place relative to the image.
@@ -985,14 +984,14 @@ function DragAndDropBlock(runtime, element, configuration) {
      * We have do this in JS, not python, since zone configurations may change.
      */
     var markItemZoneAlign = function() {
-        var zone_align = {};
+        var zone_alignments = {};
         configuration.zones.forEach(function(zone) {
-            if (!zone.align) zone.align = '';
-            zone_align[zone.title] = zone.align;
+            if (!zone.align) zone.align = 'none';
+            zone_alignments[zone.title] = zone.align;
         });
         Object.keys(state.items).forEach(function(item_id) {
             var item = state.items[item_id];
-            item.zone_align = zone_align[item.zone] || '';
+            item.zone_align = zone_alignments[item.zone] || 'none';
         });
     };
 

drag_and_drop_v2/public/js/drag_and_drop_edit.js

@@ -15,7 +15,7 @@ function DragAndDropEditBlock(runtime, element, params) {
         return Number(value).toFixed(Number(value) == parseInt(value) ? 0 : 1);
     });
     Handlebars.registerHelper('ifeq', function(v1, v2, options) {
-      if(v1 === v2) {
+      if (v1 === v2) {
         return options.fn(this);
       }
       return options.inverse(this);

drag_and_drop_v2/templates/html/js_templates.html

@@ -55,27 +55,27 @@
                 {{i18n "Alignment"}}
             </label>
             <select id="zone-{{index}}-align"
-                   class="align-select"
-                   aria-describedby="zone-align-description" >
+                    class="align-select"
+                    aria-describedby="zone-align-description">
                 <option value="" 
-                    {{#ifeq align "" }}selected{{/ifeq}}>
+                    {{#ifeq align ""}}selected{{/ifeq}}>
                     {{i18n "none"}}
                 </option>
                 <option value="left" 
-                    {{#ifeq align "left" }}selected{{/ifeq}}>
-                    {{i18n "left" }}
+                    {{#ifeq align "left"}}selected{{/ifeq}}>
+                    {{i18n "left"}}
                 </option>
                 <option value="center" 
-                    {{#ifeq align "center" }}selected{{/ifeq}}>
-                    {{i18n "center" }}
+                    {{#ifeq align "center"}}selected{{/ifeq}}>
+                    {{i18n "center"}}
                 </option>
                 <option value="right"
-                    {{#ifeq align "right" }}selected{{/ifeq}}>
-                    {{i18n "right" }}
+                    {{#ifeq align "right"}}selected{{/ifeq}}>
+                    {{i18n "right"}}
                 </option>
             </select>
             <div id="zone-align-description" class="zones-form-help">
-                {{i18n "Align dropped items to the left, center, or right.  Default is no alignment."}}
+                {{i18n "Align dropped items to the left, center, or right.  Default is no alignment (items stay exactly where the user drops them)."}}
             </div>
         </div>
     </div>

tests/integration/data/test_zone_align.json

@@ -160,7 +160,7 @@
       },
       "zone": "Zone Right Align",
       "imageURL": "",
-      "id":10 
+      "id": 10 
     },
     {
       "displayName": "DDDD",
@@ -207,6 +207,4 @@
     "start": "Intro",
     "finish": "Final"
   },
-  "targetImg": "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSI4MDAiIGhlaWdodD0iNjAwIiBzdHlsZT0iYmFja2dyb3VuZDogI2VlZjsiPjwvc3ZnPg==",
-  "targetImgDescription": "This describes the target image"
 }

tests/integration/test_interaction.py

@@ -602,15 +602,15 @@ class ZoneAlignInteractionTest(InteractionTestBase, BaseIntegrationTest):
         Ensure that they are children of the zone.
         """
         # parent container has the expected alignment
-        zone_selector = "div[data-zone='%s'] .item-align" % zone_id
-        self.assertEquals(self._get_style(zone_selector, 'textAlign'), align)
+        item_wrapper_selector = "div[data-zone='{zone_id}'] .item-wrapper".format(zone_id=zone_id)
+        self.assertEquals(self._get_style(item_wrapper_selector, 'textAlign'), align)
 
         # Items placed in zones with align setting are children of the zone
-        zone_item = '%s .option' % zone_selector
-        prev_placed_items = self._page.find_elements_by_css_selector(zone_item)
+        zone_item_selector = '%s .option' % item_wrapper_selector
+        prev_placed_items = self._page.find_elements_by_css_selector(zone_item_selector)
 
         self.place_item(item_id, zone_id, action_key)
-        placed_items = self._page.find_elements_by_css_selector(zone_item)
+        placed_items = self._page.find_elements_by_css_selector(zone_item_selector)
         self.assertEquals(len(placed_items), len(prev_placed_items) + 1)
 
         # Not children of the target
@@ -618,17 +618,17 @@ class ZoneAlignInteractionTest(InteractionTestBase, BaseIntegrationTest):
         self.assertEquals(len(self._page.find_elements_by_css_selector(target_item)), 0)
 
         # Aligned items are relative positioned, with no transform or top/left
-        self.assertEquals(self._get_style(zone_item, 'position'), 'relative')
-        self.assertEquals(self._get_style(zone_item, 'transform'), 'none')
-        self.assertEquals(self._get_style(zone_item, 'left'), '0px')
-        self.assertEquals(self._get_style(zone_item, 'top'), '0px')
+        self.assertEquals(self._get_style(zone_item_selector, 'position'), 'relative')
+        self.assertEquals(self._get_style(zone_item_selector, 'transform'), 'none')
+        self.assertEquals(self._get_style(zone_item_selector, 'left'), '0px')
+        self.assertEquals(self._get_style(zone_item_selector, 'top'), '0px')
 
         # Center-aligned items are display block
         if align == 'center':
-            self.assertEquals(self._get_style(zone_item, 'display'), 'block')
+            self.assertEquals(self._get_style(zone_item_selector, 'display'), 'block')
         # but other aligned items are just inline-block
         else:
-            self.assertEquals(self._get_style(zone_item, 'display'), 'inline-block')
+            self.assertEquals(self._get_style(zone_item_selector, 'display'), 'inline-block')
 
     def test_no_zone_align(self):
         """
@@ -637,18 +637,18 @@ class ZoneAlignInteractionTest(InteractionTestBase, BaseIntegrationTest):
         """
         zone_id = "Zone No Align"
         self.place_item(0, zone_id)
-        zone_item = "div[data-zone='%s'] .item-align .option" % zone_id
-        self.assertEquals(len(self._page.find_elements_by_css_selector(zone_item)), 0)
+        zone_item_selector = "div[data-zone='{zone_id}'] .item-wrapper .option".format(zone_id=zone_id)
+        self.assertEquals(len(self._page.find_elements_by_css_selector(zone_item_selector)), 0)
 
-        target_item = '.target > .option'
-        placed_item = self._page.find_elements_by_css_selector(target_item)
-        self.assertEquals(len(placed_item), 1)
-        self.assertEquals(placed_item[0].get_attribute('data-value'), '0')
+        target_item_selector = '.target > .option'
+        placed_items = self._page.find_elements_by_css_selector(target_item_selector)
+        self.assertEquals(len(placed_items), 1)
+        self.assertEquals(placed_items[0].get_attribute('data-value'), '0')
 
         # Non-aligned items are absolute positioned, with top/bottom set to px
-        self.assertEquals(self._get_style(target_item, 'position'), 'absolute')
-        self.assertRegexpMatches(self._get_style(target_item, 'left'), r'^\d+(\.\d+)?px$')
-        self.assertRegexpMatches(self._get_style(target_item, 'top'), r'^\d+(\.\d+)?px$')
+        self.assertEquals(self._get_style(target_item_selector, 'position'), 'absolute')
+        self.assertRegexpMatches(self._get_style(target_item_selector, 'left'), r'^\d+(\.\d+)?px$')
+        self.assertRegexpMatches(self._get_style(target_item_selector, 'top'), r'^\d+(\.\d+)?px$')
 
     def test_zone_align_invalid(self):
         """
@@ -697,20 +697,13 @@ class ZoneAlignInteractionTest(InteractionTestBase, BaseIntegrationTest):
         self._assert_zone_align_item(9, 'Zone Right Align', 'right', Keys.RETURN)
         self._assert_zone_align_item(10, 'Zone Right Align', 'right', Keys.RETURN)
         self._assert_zone_align_item(11, 'Zone Right Align', 'right', Keys.RETURN)
-    '''
-    FIXME Erratic bug in place_item(action_key=None) when there's too many zones?
-    Works consistently from keyboard and manual testing.
-    def test_zone_align_center(self):
-        """
-        Test items placed in a zone with an center align setting.
-        """
-        self._assert_zone_align_item(12, 'Zone Center Align', 'center')
-        self._assert_zone_align_item(13, 'Zone Center Align', 'center')
-        self._assert_zone_align_item(14, 'Zone Center Align', 'center')
-    '''
+
     def test_zone_align_center_key(self):
         """
         Test items placed in a zone with an center align setting, via keyboard.
+
+        note: Erratic bug in place_item(action_key=None) when there's too many zones?
+        Works consistently from keyboard and manual testing.
         """
         self._assert_zone_align_item(12, 'Zone Center Align', 'center', Keys.RETURN)
         self._assert_zone_align_item(13, 'Zone Center Align', 'center', Keys.RETURN)

tests/integration/test_render.py

@@ -181,7 +181,7 @@ class TestDragAndDropRender(BaseIntegrationTest):
             self.assertEqual(zone.get_attribute('dropzone'), 'move')
             self.assertEqual(zone.get_attribute('aria-dropeffect'), 'move')
             self.assertEqual(zone.get_attribute('data-zone'), 'Zone {}'.format(zone_number))
-            self.assertEqual(zone.get_attribute('data-zone_align'), '')
+            self.assertEqual(zone.get_attribute('data-zonealign'), 'none')
             self.assertIn('ui-droppable', self.get_element_classes(zone))
             zone_box_percentages = box_percentages[index]
             self._assert_box_percentages(  # pylint: disable=star-args
@@ -283,13 +283,14 @@ class TestDragAndDropRenderZoneAlign(BaseIntegrationTest):
         self._page = self.go_to_page(self.PAGE_TITLE)
 
     def test_zone_align(self):
-        self.assertEquals(self._get_style('#-zone-none .item-wrapper', 'textAlign'), 'start')
-        self.assertEquals(self._get_style('#-zone-none .item-wrapper', 'textAlign', True), 'start')
-        self.assertEquals(self._get_style('#-zone-invalid .item-wrapper', 'textAlign'), 'start')
-        self.assertEquals(self._get_style('#-zone-invalid .item-wrapper', 'textAlign', True), 'start')
-        self.assertEquals(self._get_style('#-zone-left .item-wrapper', 'textAlign'), 'left')
-        self.assertEquals(self._get_style('#-zone-left .item-wrapper', 'textAlign', True), 'left')
-        self.assertEquals(self._get_style('#-zone-right .item-wrapper', 'textAlign'), 'right')
-        self.assertEquals(self._get_style('#-zone-right .item-wrapper', 'textAlign', True), 'right')
-        self.assertEquals(self._get_style('#-zone-center .item-wrapper', 'textAlign'), 'center')
-        self.assertEquals(self._get_style('#-zone-center .item-wrapper', 'textAlign', True), 'center')
+        expected_alignments = {
+            "#-zone-none": "start",
+            "#-zone-invalid": "start",
+            "#-zone-left": "left",
+            "#-zone-right": "right",
+            "#-zone-center": "center"
+        }
+        for zone_id, expected_alignment in expected_alignments.items():
+            selector = "{zone_id} .item-wrapper".format(zone_id=zone_id)
+            self.assertEquals(self._get_style(selector, "textAlign"), expected_alignment)
+            self.assertEquals(self._get_style(selector, "textAlign", computed=True), expected_alignment)