Implement Pagination for Content libraries (SOL-4) #6163
Conversation
Kelketek
commented
Dec 5, 2014
Kelketek
commented
- Background: This PR provides pagination for content Libraries in the container view, when displaying the blocks contained in the library.
- Jira tickets: Implements SOL-4
- Discussions: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up.
- Dependencies: https://github.com/edx/edx-platform/pull/6046 (merged, in feature branch https://github.com/edx/edx-platform/commits/content-libraries)
- Sandbox URL: http://sandbox.opencraft.com:18010/library/library-v1:TST-X-01+now
- Internal code reviews PRs: Content libraries paging open-craft/edx-platform#9 Implement Pagination for Content libraries open-craft/edx-platform#18
- Partner information: 3rd party-hosted open edX instance, for an edX solutions client.
|
Thanks for the pull request, @Kelketek! I've created OSPR-261 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
Kelketek
changed the title
|
@sarina This should be ready for review now-- removed the WIP flag, but I'm not sure what to do about Jenkins' quality report, since it's flagging a couple of 'TODO' comments, both of which are not intended to be addressed in this PR but which we don't really want to forget either. |
antoviaque
changed the title
|
Sorry @sarina , just found out you're not the person to ping for this. :) @explorerleslie @cahrens , this is ready for your review, and a sandbox is available, linked in the description with the standard |
|
@Kelketek this PR will help the TODO issue: https://github.com/edx/edx-platform/pull/6122 |
|
When you add a new component to the page, it is currently scrolling to show the top of the page. It should instead scroll down to show the newly added component. Related to this, it seems like the entire page is re-rendering each time a component is added, which does not seem necessary. Adding a new component will either case you go to go the next page (if at least 10 components already exist on the page), or it will be added as the last component on the current page. It seems like you can easily avoid re-rendering in the 2nd case. (also note that I screwed up Xavier's test library by adding a discussion component-- I am so used to just clicking anything and forgot that it isn't expected to work) |
|
Drag and drop is now disabled. Is that intentional? |
| # Set up the context to be passed to each XBlock's render method. | ||
| context = { | ||
| 'is_pages_view': is_pages_view, # This setting disables the recursive wrapping of xblocks | ||
| 'is_unit_page': is_unit(xblock), | ||
| 'root_xblock': xblock if (view_name == 'container_preview') else None, | ||
| 'reorderable_items': reorderable_items | ||
| 'reorderable_items': reorderable_items, | ||
| 'paging': paging |
There was a problem hiding this comment.
Can you add a trailing comma here, so that if something else needs to be added to the context, this line won't have to change again?
|
@singingwolfboy Certainly! I'll get right on that. |
|
@cahrens Yes, drag-and-drop's disabling is intentional, as I remember-- I can try to find the historical conversation on it, if you wish. |
|
@singingwolfboy Wait, if I rebase on top of master with that, won't a merge with this feature branch be unclean? The feature branch would need a rebase first. Has it been? |
|
@Kelketek no, rebasing a branch makes it more clean than merging master into it. See the How to Rebase a Pull Request page for more info. |
|
I think the feature branch should be rebased on master, then this branch rebased on the feature branch. |
|
@singingwolfboy That's not what I mean-- This branch is not intended to be merged into master. It's intended to be merged into the |
|
Oh! I totally missed that, sorry. We'll need to get the |
|
@singingwolfboy What's the procedure for that? Does one just create a new branch with the rebase, and someone on your end does a reset --hard against it? |
|
@Kelketek yes, that's basically the procedure. I'll take care of it -- I just want to make sure that everyone within edX (who cares about this branch) is aware of the change. |
|
@singingwolfboy Thank you. I'll let the others at OpenCraft know that the feature branch will be rebased, and to prepare accordingly, since that means they may have to discard commits that aren't theirs on their rebase against the feature branch. |
|
@Kelketek Copy-paste makes me cringe. Should library-container view extend the container view? |
|
@cahrens I've just went through that file again - there're like two or three methods that can be shared. So it's not good and shame on me (since I have implemented it initially), but that's not that bad to cringe :) But of course it's useful to factor it out so that those methods are shared. On the other hand, if library container extend ordinary container there would be more overloads than shared methods, so it might make sense to have a common ancestor, but not making library-container a descendant of ordinary container. |
|
From a functionality perspective, extension seems like the right thing here-- a library is a container page that adds additional features like paging. |
| category: 'vertical' | ||
| }); | ||
| }); | ||
| function parameterized_suite(label, global_page_options, fixtures) { |
There was a problem hiding this comment.
Can you give a high-level description of what changed in this file? I'm having trouble understanding via the diff.
There was a problem hiding this comment.
@cahrens parameterized the whole test suite. It basically now works in two modes: with ordinary container and with library container. It tests common functionality found in both of them. The change was to wrap entire test suite into a function with some parameters and call it multiple times. As a result, every line changed indentation, and github diff viewer is unable to detect that, hence such a terrible diff.
There was a problem hiding this comment.
if you add ?w=1 to the end of the URL, Github will ignore whitespace when calculating diffs. That might make this easier to read.
openedx-webhooks
added
awaiting prioritization
engineering review
and removed
awaiting prioritization
labels
Kelketek
force-pushed
the
content_libraries/4-pagination
branch
from
22f240f
to
cbd7e9b
Compare
Kelketek
force-pushed
the
content_libraries/4-pagination
branch
from
cbd7e9b
to
84e3183
Compare
|
@cahrens This is ready for another reviewing pass. Jenkins is still barfing on the quality checks even post-rebase, and it is now freaking out on bok_choy, heaven knows why, but I can't reproduce that locally either. |
|
OK. I don't think I will be able to get back to this until Monday or Tuesday. |
|
Updated the sandbox with the latest code: http://sandbox.opencraft.com:18010/ |
| for _ in range(0, 10): | ||
| add_component(self.lib_page, "problem", "Multiple Choice") | ||
| self.assertEqual(len(self.lib_page.xblocks), 10) | ||
| add_component(self.lib_page, "problem", "Multiple Choice") |
There was a problem hiding this comment.
I still think it would be good to make the last component unique in some way, and verify that the one displayed on the new page is the 11th one you added.
Actually, I think this is covered below, so I don't feel as strongly about it.
|
👍 Just a few nits, but otherwise good. |
Kelketek
force-pushed
the
content_libraries/4-pagination
branch
from
84e3183
to
be3371e
Compare
|
@antoviaque Jenkins has finished. The only failure is that unreproducible quality check issue as before. |
Implement Pagination for Content libraries (SOL-4)
Implement Pagination for Content libraries (SOL-4)
