Replace the unit page with the container page #4136
Conversation
|
Updated PR for https://github.com/edx/edx-platform/pull/4116 that compares against the bulk-publishing branch. |
andy-armstrong
changed the title
|
@explorerleslie @cahrens @frrrances I've made the changes we discussed this morning so I've put this up on my sandbox. I also created a test course that has both published and unpublished changes since you can't make those changes now that the publishing sidebar has gone! http://studio.andy-armstrong.m.sandbox.edx.org/course/slashes:AndyA+BP101+BP101 |
|
@andy-armstrong Breadcrumbs are getting truncated.
|
|
@frrrances Subsections in the RHS "unit locator" thingy shouldn't look clickable. Right now they do, though Andy has correct disabled navigation to the Subsection page. |
|
@cahrens the breadcrumbs appear to be generated with the correct HTML so @frrrances was investigating the styling. |
|
@cahrens Thanks for catching this. I've made the changes we discussed so that the special logic for static pages is clearly pointed out. |
|
@frrrances FYI, you can remove view-unit from the sass now, as that view has gone entirely. |
There was a problem hiding this comment.
Let's rename as xblock_input_editor and be clear that it is only for strings.
There was a problem hiding this comment.
Or if you want to move to generalizing it, create something like the abstract editor class with getValue, setValue, hideEditor, and showEditor methods. Then have a StringEditor implementation of it that deals with the input field.
|
@frrrances can _unit.scss be deleted? |
There was a problem hiding this comment.
Note to reviewer: I deleted the Lettuce tests around publishing and creating drafts. I will write tests for the new workflow in bok choy.
|
@andy-armstrong and/or @frrrances Styling of editor for the video component is broken. They are doing custom tab work, so probably their CSS is no longer getting applied on the "container page".
|
|
The upload dialog within the Video editor has issues as well. It is supposed to be small enough that you can work with it, but now it is large and scrolling causes it and the settings editor behind to move. Steps to reproduce: Create video, click "Advanced", click on "Upload Handout". |
There was a problem hiding this comment.
I'm not sure read_only is necessary here. But I guess it doesn't hurt anything.
There was a problem hiding this comment.
There are templates that assume it is set, and I thought this was simpler than updating them all to be tolerant of the missing value. Would you prefer that I did?
|
@andy-armstrong Done reviewing. |
|
👍 for your changes, @cahrens. Thanks for helping me get through all those test failures. I'm done with addressing your review comments too, btw. |
|
I've squashed our code review changes (and corrected the issue I introduced a couple commits ago). 👍 on your stuff, Andy. |
|
@andy-armstrong On the video editor displaying incorrectly: it looks like the model-type-[type] is not getting populated correctly. Once the class reads "modal-type-video" it seems to fix both the editor window and the Upload Handout dialog. Let me know if you need help with getting that fixed. |
|
@cahrens @frrrances I've fixed the video editing bug (somehow the key line of code that set the category of the block got removed). I've also implemented the title logic for container pages. Please re-review just my last two commits. |
|
@marcotuts Can you review my FED and UX work on this? |
|
@andy-armstrong Video looks much better! 👍 |
There was a problem hiding this comment.
should this be our edX white/off-white as opposed to browser white? (nitpick, skippable)
|
I've updated my Sandbox and added a test course: http://studio.andy-armstrong.m.sandbox.edx.org/course/slashes:AndyA+AAT101+AAT101 |
|
This looks great, thanks @andy-armstrong and @frrrances! Two minor pieces of feedback (both of which I think are targeted to @andy-armstrong):
|
|
Awesome! Glad to see this work to replace the unit page. A couple thoughts (after chatting with Frances to make sure I caught everything included in this work), but nothing that would stop this from merging. 1.) For the breadcrumb navigation, I think it would be good to find a way to make all of those items clickable, including the subsection pages (eventually). This could come in the form of an outline page link anchored to the relevant Section or Subsection, but this way all of these links can be used consistently as navigation. 2.) Additional iconography or hover states for the editable Unit page name would be a helpful improvement, though it sounds like that is already planned for. Look great! 👍 |
|
@explorerleslie |
|
We need to leave display_name in the edit modal. As Andy alluded to, there is no way for us to tell on the server at the time we create the information for the settings modal where it will be displayed. |
|
@cahrens thanks, Andy and I talked about that and I'm ok with leaving it as is for now. |
|
Uck, I lost this commit when I tried to rebase bulk-publishing onto master. I will create a new branch and PR with the cherry-picked commit. Need to figure out how to do this better next time! |
|
New PR is #4245 |
STUD-1754