From 80c517ecd1c8eb3e327ee67f7d827751d26a20c3 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Mon, 8 Dec 2014 22:22:02 +0000 Subject: [PATCH] Addressed notes from reviewers on Library Pagination. --- cms/djangoapps/contentstore/views/item.py | 5 +- .../js/spec/views/pages/container_spec.js | 32 +- cms/static/js/views/library_container.js | 71 +++-- cms/static/js/views/pages/container.js | 23 +- cms/static/sass/elements/_pagination.scss | 8 +- cms/static/sass/elements/_xblocks.scss | 2 +- ...ontainer-paged-after-add-xblock.underscore | 283 ------------------ .../js/mock/mock-xblock-paged.underscore | 21 ++ .../xmodule/xmodule/library_root_xblock.py | 3 +- .../xmodule/video_module/video_handlers.py | 1 + 10 files changed, 113 insertions(+), 336 deletions(-) delete mode 100644 cms/templates/js/mock/mock-container-paged-after-add-xblock.underscore create mode 100644 cms/templates/js/mock/mock-xblock-paged.underscore diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index e46d83d70b..bd2a57e1a9 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -206,6 +206,7 @@ def xblock_view_handler(request, usage_key_string, view_name): store = modulestore() xblock = store.get_item(usage_key) container_views = ['container_preview', 'reorderable_container_child_preview'] + library = isinstance(usage_key, LibraryUsageLocator) # wrap the generated fragment in the xmodule_editor div so that the javascript # can bind to it correctly @@ -234,7 +235,7 @@ def xblock_view_handler(request, usage_key_string, view_name): # are being shown in a reorderable container, so the xblock is automatically # added to the list. reorderable_items = set() - if view_name == 'reorderable_container_child_preview': + if not library and view_name == 'reorderable_container_child_preview': reorderable_items.add(xblock.location) paging = None @@ -258,7 +259,7 @@ def xblock_view_handler(request, usage_key_string, view_name): 'is_unit_page': is_unit(xblock), 'root_xblock': xblock if (view_name == 'container_preview') else None, 'reorderable_items': reorderable_items, - 'paging': paging + 'paging': paging, } fragment = get_preview_fragment(request, xblock, context) diff --git a/cms/static/js/spec/views/pages/container_spec.js b/cms/static/js/spec/views/pages/container_spec.js index 6f4b4baf46..d5ec6938dc 100644 --- a/cms/static/js/spec/views/pages/container_spec.js +++ b/cms/static/js/spec/views/pages/container_spec.js @@ -273,7 +273,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel }); describe("xblock operations", function () { - var getGroupElement, + var getGroupElement, paginated, NUM_COMPONENTS_PER_GROUP = 3, GROUP_TO_TEST = "A", allComponentsInGroup = _.map( _.range(NUM_COMPONENTS_PER_GROUP), @@ -282,6 +282,11 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel } ); + paginated = function () { + return containerPage.enable_paging; + }; + + getGroupElement = function () { return containerPage.$("[data-locator='locator-group-" + GROUP_TO_TEST + "']"); }; @@ -294,6 +299,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel promptSpy = EditHelpers.createPromptSpy(); }); + clickDelete = function (componentIndex, clickNo) { // find all delete buttons for the given group @@ -307,21 +313,25 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel EditHelpers.confirmPrompt(promptSpy, clickNo); }; - deleteComponent = function (componentIndex) { + deleteComponent = function (componentIndex, requestOffset) { clickDelete(componentIndex); AjaxHelpers.respondWithJson(requests, {}); // second to last request contains given component's id (to delete the component) AjaxHelpers.expectJsonRequest(requests, 'DELETE', '/xblock/locator-component-' + GROUP_TO_TEST + (componentIndex + 1), - null, requests.length - 2); + null, requests.length - requestOffset); // final request to refresh the xblock info AjaxHelpers.expectJsonRequest(requests, 'GET', '/xblock/locator-container'); }; deleteComponentWithSuccess = function (componentIndex) { - deleteComponent(componentIndex); + var deleteOffset; + + deleteOffset = paginated() ? 3 : 2; + + deleteComponent(componentIndex, deleteOffset); // verify the new list of components within the group expectComponents( @@ -350,9 +360,16 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel containerPage.$('.delete-button').first().click(); EditHelpers.confirmPrompt(promptSpy); AjaxHelpers.respondWithJson(requests, {}); + var deleteOffset; + + if (paginated()) { + deleteOffset = 3; + } else { + deleteOffset = 2; + } // expect the second to last request to be a delete of the xblock AjaxHelpers.expectJsonRequest(requests, 'DELETE', '/xblock/locator-broken-javascript', - null, requests.length - 2); + null, requests.length - deleteOffset); // expect the last request to be a fetch of the xblock info for the parent container AjaxHelpers.expectJsonRequest(requests, 'GET', '/xblock/locator-container'); }); @@ -511,7 +528,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel }); describe('Template Picker', function () { - var showTemplatePicker, verifyCreateHtmlComponent; + var showTemplatePicker, verifyCreateHtmlComponent, call_count; showTemplatePicker = function () { containerPage.$('.new-component .new-component-type a.multiple-templates')[0].click(); @@ -519,6 +536,7 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel verifyCreateHtmlComponent = function (test, templateIndex, expectedRequest) { var xblockCount; + // call_count = paginated() ? 18: 10; renderContainerPage(test, mockContainerXBlockHtml); showTemplatePicker(); xblockCount = containerPage.$('.studio-xblock-wrapper').length; @@ -557,6 +575,6 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel { enable_paging: true, page_size: 42 }, { initial: 'mock/mock-container-paged-xblock.underscore', - add_response: 'mock/mock-container-paged-after-add-xblock.underscore' + add_response: 'mock/mock-xblock-paged.underscore' }); }); diff --git a/cms/static/js/views/library_container.js b/cms/static/js/views/library_container.js index b655833289..a8c15999ab 100644 --- a/cms/static/js/views/library_container.js +++ b/cms/static/js/views/library_container.js @@ -1,19 +1,16 @@ -define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", "js/views/feedback_notification", +define(["jquery", "underscore", "js/views/container", "js/utils/module", "gettext", "js/views/feedback_notification", "js/views/paging_header", "js/views/paging_footer"], - function ($, _, XBlockView, ModuleUtils, gettext, NotificationView, PagingHeader, PagingFooter) { - var LibraryContainerView = XBlockView.extend({ + function ($, _, ContainerView, ModuleUtils, gettext, NotificationView, PagingHeader, PagingFooter) { + var LibraryContainerView = ContainerView.extend({ // Store the request token of the first xblock on the page (which we know was rendered by Studio when // the page was generated). Use that request token to filter out user-defined HTML in any // child xblocks within the page. - requestToken: "", initialize: function(options){ var self = this; - XBlockView.prototype.initialize.call(this); + ContainerView.prototype.initialize.call(this); this.page_size = this.options.page_size || 10; - if (options) { - this.page_reload_callback = options.page_reload_callback; - } + this.page_reload_callback = options.page_reload_callback || function () {}; // emulating Backbone.paginator interface this.collection = { currentPage: 0, @@ -30,9 +27,6 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", render: function(options) { var eff_options = options || {}; - if (eff_options.block_added) { - this.collection.currentPage = this.getPageCount(this.collection.totalCount+1) - 1; - } eff_options.page_number = typeof eff_options.page_number !== "undefined" ? eff_options.page_number : this.collection.currentPage; @@ -53,9 +47,8 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", success: function(fragment) { self.handleXBlockFragment(fragment, options); self.processPaging({ requested_page: options.page_number }); - if (options.paging && self.page_reload_callback){ - self.page_reload_callback(self.$el); - } + // This is expected to render the add xblock components menu. + self.page_reload_callback(self.$el) } }); }, @@ -69,12 +62,12 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", }, getPageCount: function(total_count){ - if (total_count==0) return 1; + if (total_count===0) return 1; return Math.ceil(total_count / this.page_size); }, setPage: function(page_number) { - this.render({ page_number: page_number, paging: true }); + this.render({ page_number: page_number}); }, nextPage: function() { @@ -129,32 +122,54 @@ define(["jquery", "underscore", "js/views/xblock", "js/utils/module", "gettext", }, xblockReady: function () { - XBlockView.prototype.xblockReady.call(this); + ContainerView.prototype.xblockReady.call(this); this.requestToken = this.$('div.xblock').first().data('request-token'); }, - refresh: function() { }, + refresh: function(block_added) { + if (block_added) { + this.collection.totalCount += 1; + this.collection._size +=1; + if (this.collection.totalCount == 1) { + this.render(); + return + } + this.collection.totalPages = this.getPageCount(this.collection.totalCount); + var new_page = this.collection.totalPages - 1; + // If we're on a new page due to overflow, or this is the first item, set the page. + if (((this.collection.currentPage) != new_page) || this.collection.totalCount == 1) { + this.setPage(new_page); + } else { + this.pagingHeader.render(); + this.pagingFooter.render(); + } + } + }, acknowledgeXBlockDeletion: function (locator){ this.notifyRuntime('deleted-child', locator); this.collection._size -= 1; this.collection.totalCount -= 1; - // pages are counted from 0 - thus currentPage == 1 if we're on second page - if (this.collection._size == 0 && this.collection.currentPage >= 1) { - this.setPage(this.collection.currentPage - 1); - this.collection.totalPages -= 1; - } - else { + var current_page = this.collection.currentPage; + var total_pages = this.getPageCount(this.collection.totalCount); + this.collection.totalPages = total_pages; + // Starts counting from 0 + if ((current_page + 1) > total_pages) { + // The number of total pages has changed. Move down. + // Also, be mindful of the off-by-one. + this.setPage(total_pages - 1) + } else if ((current_page + 1) != total_pages) { + // Refresh page to get any blocks shifted from the next page. + this.setPage(current_page) + } else { + // We're on the last page, just need to update the numbers in the + // pagination interface. this.pagingHeader.render(); this.pagingFooter.render(); } }, - makeRequestSpecificSelector: function(selector) { - return 'div.xblock[data-request-token="' + this.requestToken + '"] > ' + selector; - }, - sortDisplayName: function() { return "Date added"; // TODO add support for sorting } diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index c63e7f00f6..771e9afb1b 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -119,8 +119,11 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views // Notify the runtime that the page has been successfully shown xblockView.notifyRuntime('page-shown', self); - // Render the add buttons - self.renderAddXBlockComponents(); + // Render the add buttons. Paged containers should do this on their own. + if (!self.enable_paging) { + // Render the add buttons + self.renderAddXBlockComponents(); + } // Refresh the views now that the xblock is visible self.onXBlockRefresh(xblockView); @@ -141,8 +144,8 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views return this.xblockView.model.urlRoot; }, - onXBlockRefresh: function(xblockView) { - this.xblockView.refresh(); + onXBlockRefresh: function(xblockView, block_added) { + this.xblockView.refresh(block_added); // Update publish and last modified information from the server. this.model.fetch(); }, @@ -274,10 +277,10 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views rootLocator = this.xblockView.model.id; if (xblockElement.length === 0 || xblockElement.data('locator') === rootLocator) { this.render({refresh: true, block_added: block_added}); - } else if (parentElement.hasClass('reorderable-container')) { - this.refreshChildXBlock(xblockElement); + } else if (parentElement.hasClass('reorderable-container') || this.enable_paging) { + this.refreshChildXBlock(xblockElement, block_added); } else { - this.refreshXBlock(this.findXBlockElement(parentElement), block_added); + this.refreshXBlock(this.findXBlockElement(parentElement)); } }, @@ -285,9 +288,11 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views * Refresh an xblock element inline on the page, using the specified xblockInfo. * Note that the element is removed and replaced with the newly rendered xblock. * @param xblockElement The xblock element to be refreshed. + * @param block_added Specifies if a block has been added, rather than just needs + * refreshing. * @returns {jQuery promise} A promise representing the complete operation. */ - refreshChildXBlock: function(xblockElement) { + refreshChildXBlock: function(xblockElement, block_added) { var self = this, xblockInfo, TemporaryXBlockView, @@ -313,7 +318,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }); return temporaryView.render({ success: function() { - self.onXBlockRefresh(temporaryView); + self.onXBlockRefresh(temporaryView, block_added); temporaryView.unbind(); // Remove the temporary view } }); diff --git a/cms/static/sass/elements/_pagination.scss b/cms/static/sass/elements/_pagination.scss index f3ba465b80..379d8785e3 100644 --- a/cms/static/sass/elements/_pagination.scss +++ b/cms/static/sass/elements/_pagination.scss @@ -2,7 +2,7 @@ // ========================== %pagination { - @include clearfix; + @include clearfix(); display: inline-block; width: flex-grid(3, 12); @@ -48,7 +48,7 @@ } .nav-label { - @extend .sr; + @extend %cont-text-sr; } .pagination-form, @@ -89,7 +89,7 @@ .page-number-label, .submit-pagination-form { - @extend .sr; + @extend %cont-text-sr; } .page-number-input { @@ -116,4 +116,4 @@ } } } -} \ No newline at end of file +} diff --git a/cms/static/sass/elements/_xblocks.scss b/cms/static/sass/elements/_xblocks.scss index 66b1e603a6..68298caaf8 100644 --- a/cms/static/sass/elements/_xblocks.scss +++ b/cms/static/sass/elements/_xblocks.scss @@ -105,7 +105,7 @@ .container-paging-header { .meta-wrap { - margin: $baseline $baseline/2; + margin: $baseline ($baseline/2); } .meta { @extend %t-copy-sub2; diff --git a/cms/templates/js/mock/mock-container-paged-after-add-xblock.underscore b/cms/templates/js/mock/mock-container-paged-after-add-xblock.underscore deleted file mode 100644 index cb260c9bca..0000000000 --- a/cms/templates/js/mock/mock-container-paged-after-add-xblock.underscore +++ /dev/null @@ -1,283 +0,0 @@ - -
-
-
- Test Container -
-
-
    -
-
-
-
-
-
- - - - -
- -
-
-
-
-
- - - Expand or Collapse - - Group A -
-
-
    -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- - - Expand or Collapse - - Group B -
-
-
    -
  • - -
    • -
-
-
-
- -
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
    -
  • - -
    • -
  • - -
    • -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
-
-
-
-
-
-
-
-
diff --git a/cms/templates/js/mock/mock-xblock-paged.underscore b/cms/templates/js/mock/mock-xblock-paged.underscore new file mode 100644 index 0000000000..c6c2c881d8 --- /dev/null +++ b/cms/templates/js/mock/mock-xblock-paged.underscore @@ -0,0 +1,21 @@ +
+
+
+ Mock XBlock +
+
+
    +
+ + Add Missing Groups + +
+
+
+
+

Mock XBlock

+
+
+
diff --git a/common/lib/xmodule/xmodule/library_root_xblock.py b/common/lib/xmodule/xmodule/library_root_xblock.py index 497a145b79..3118f9a258 100644 --- a/common/lib/xmodule/xmodule/library_root_xblock.py +++ b/common/lib/xmodule/xmodule/library_root_xblock.py @@ -76,7 +76,7 @@ class LibraryRoot(XBlock): fragment.add_frag_resources(rendered_child) contents.append({ - 'id': child.location.to_deprecated_string(), + 'id': unicode(child.location), 'content': rendered_child.content }) @@ -85,7 +85,6 @@ class LibraryRoot(XBlock): 'items': contents, 'xblock_context': context, 'can_add': can_add, - 'can_reorder': False, 'first_displayed': item_start, 'total_children': children_count, 'displayed_children': len(children_to_show) diff --git a/common/lib/xmodule/xmodule/video_module/video_handlers.py b/common/lib/xmodule/xmodule/video_module/video_handlers.py index 1ba427c357..9e9db860ca 100644 --- a/common/lib/xmodule/xmodule/video_module/video_handlers.py +++ b/common/lib/xmodule/xmodule/video_module/video_handlers.py @@ -155,6 +155,7 @@ class VideoStudentViewHandlers(object): if transcript_name: # Get the asset path for course + asset_path = None course = self.descriptor.runtime.modulestore.get_course(self.course_id) if course.static_asset_path: asset_path = course.static_asset_path