-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Edit section and subsection at course outline page. #4425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a separate asset call for this specific page is less than ideal (especially with us using this vendor datepicker UI on other Studio pages, e.g. Course Settings). Its fine for now, but a follow-up story might be worthwhile to better bundle our vendor assets together with our styling for optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ @andy-armstrong, do you agree here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talbs you're right, it makes sense.
|
@auraz, just a heads up - when opening the settings modal on a Section, the title of the modal says "Subsection Settings". This should be changed to "Section Settings". Here's a screengrab to help understand the issue: |
|
@frrrances, can you review my UI and FED work here once more for final UX sign off? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is already in the "specific modal overrides" section, do you want to make this header describe this particular modal type, like "section settings modal"?
|
@talbs Nice work! I had a couple small questions on the code above. My only other comment is that the spacing is pretty big in the modal; I would tighten it up, but if you are happy as-is, I'm cool with that. 👍 |
|
Thanks, @frrrances. I've refactored the UI for the outline item editor modal and new content-based modal styles into the general modal Sass partial. I've also tried to reduce the visual space around elements in the modal. Thanks for the recommendations. |
|
Could the dialogs have the name of the Section or Subsection in them so that you can be sure you are editing for the correct thing? @talbs, what do you think? |
|
I am seeing a bug where setting the grading type on a subsection causes the subsection to collapse (hide all the units). Repro steps-
This happens to me for setting dates as well, and changes on the section as well as the subsection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can more of the fields you added (start, due_date, graded, due, format) be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: "Can more of the fields you added (start, due_date, graded, due, format) be tested?"
|
Why can't you clear the Section release date to go back to the default? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset grading type to Not Graded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think test had wrong description, as It makes no sense to test html select. UI does not have special button ("remove icon") to clear grading types. Possibility to change values of grading format is tested in other jasmine tests and in bok_choy tests. Fixed test description.
|
@explorerleslie it looks like we will need another story to sync up the styling of how the release date, due date, and grading type are displayed on the course outline page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: "Correct comment."
|
@cahrens, thanks for the UX/UI heads ups. Adding specific Section/Subsection names to the modal titles sounds great and I agree it should be added. |
|
@frrrances Please clarify .modal-editor issue:
Regarding defaults: |
|
@cahrens Do we need changelog for this kind of PRs, which go into bulk-publishing? |
|
@frrrances your comment is addressed, please check. |
|
@cahrens all comments are addressed, please continue review |
|
@auraz Looks good! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename isEditable to something like "isEditableOnCourseOutline"? It's confusing because verticals certainly are editable-- on the container page. The current name of the method implies a much more generic definition of editability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: "Can you rename isEditable to something like "isEditableOnCourseOutline"
|
Done with re-review-- looking good! I personally think the modal dialog is much too wide. I have pinged @frrrances about that. |
|
@cahrens all your comments were addressed. It looks bulk-publishing was rebased. So I will squash commits and rebase on it again. |
|
That were merge conflicts. Rebased. |
|
Setting the release date is causing a collapsed section to expand. The root cause is probably the same as for https://openedx.atlassian.net/browse/STUD-2038. |
|
OK, @talbs I might opt to go back to the "large" dialog. The dialog is wrapping on me, even when browser is maximized. |
|
Not quite, in this PR issue for subsections was fixed. But for sections not. Aelxander Kryklia On Wednesday, July 30, 2014 at 9:45 PM, Christina Roberts wrote:
|
|
@talbs OK, thanks |
|
👍 |
|
Failing tests: @cahrens do you think I can merge? |
Fix bok_choy test by changing course separator. Change format of the modal title to '[Subsection/Section Name] Settings'. Improve bok_choy test stability. Studio: correcting modal window size name for outline settings editor Specify full date in bok_choy tests. Refactor bok_choy tests. Remove .modal-editor from basic-modal.underscore Set classes in modal window dynamically. Studio: revising outline edit modal tip content and overall size Rename isEditable to isEditableOnCourseOutline. Interpolate display name. Use graded instead of format as flag. Studio: revising outline settings edit modal size Fix selectors in bok_choy tests.






As a course author, I want to view and change the release date and time for subsections/sections and the due date and time and grading type for subsections from the course outline page.
https://openedx.atlassian.net/browse/STUD-1881
Test instance: http://studio.auraz.m.sandbox.edx.org/course/edX/Open_DemoX/edx_demo_course
VIEWING:
EDITING:
NOTES:
staff only (overrided release date)checkbox present in mockups is not implemented (outside of the scope of the task)@cahrens @andy-armstrong @frrrances @explorerleslie @mhoeber please review.