Content libraries paging

[e-kolpakov]

No description provided.

[e-kolpakov]

container_spec.js seem heavily modified, but in reality I just wrapped entire test suite in parameterized_suite function and called it with parameters for paged and non-paged container view. There are some minor changes to allow for it, though.

[e-kolpakov]

I don't like the idea of passing data through data attributes in rendered html, but it's a workaround that allows paging to work. Otherwise some invasive modifications need to be made to the way child XBlocks are delivered to studio

[bradenmacdonald]

Yeah I see what you mean. The only thing I can suggest for now is changing the end of this line slightly since <div /> is not valid HTML5.

[bradenmacdonald]

Very nice work @e-kolpakov! Seems to work very well. Comments made inline.

The only issue I encountered is just missing functionality: right now if I go to page 3 then refresh, I find myself back on page 1. I think you should use history.pushState() so that the "current page" is represented in the URL (and also fix the page links to preventDefault() so # isn't added to the URL when they are clicked). I know that may be complicated to implement though so maybe you want to make that a future sprint task or wait until upstream has signed off on this.

As for how you chose to implement this overall, I think it's good, but we'll have to see what upstream says :)

[e-kolpakov]

@bradenmacdonald thanks for the review! I've addressed those inline review notes. As of history - unfortunately there are no Backbone routing present at where we are, so we either need to implement backbone routing or do history manipulations using plain HTML5 API.

I've spent about an hour implementing HTML5 version. In the end it was capable of tracking history and handling back button, but with bugs and zero test coverage. So I decided that it deserves a separate task.

[bradenmacdonald]

@e-kolpakov Sounds good :)

[bradenmacdonald]

This will need to be rebased onto content_libraries/2-studio-lib-support. The rebase will be complicated by the fact that I changed the library XModule to a pure XBlock, but although that will be a conflict it should make the code nicer and simpler in the end. This branch will also need bok choy tests.

Once PR 6046 has been merged, we will open an upstream PR to merge this branch into the content-libraries feature branch.

[e-kolpakov]

@bradenmacdonald just one question is content_libraries/2-studio-lib-support ready for rebasing? I mean, since it's a base for PR 6046, there will be more modifications to it while you address the comments from upstream review, so it might make sense to hold off rebasing this branch until PR 6046 is merged, to reduce the number of rebases.

[bradenmacdonald]

@e-kolpakov Yes, you might want to wait until PR 6046 is merged. Up to you.