-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Delete standalone Subsection edit page #4540
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
cms/static/js/base.js
Outdated
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.
$('a[rel="external"]').attr({
'title': gettext('This link will open in a new browser window/tab')),
'target': '_blank'
});|
@tymofij Do we need to remove styles in this PR? |
|
@cahrens, @frrrances and @andy-armstrong please take a look. |
|
@frrrances and @explorerleslie, should the subsection be clickable on the unit location sidebar (I imagine it taking you to the outline page, scrolling to the subsection)? Same question for the breadcrumb on the unit 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.
I'm wondering if this should be clickable (going to the outline page).
|
Love, love, love all the deleted code. Of course, I can't say if MORE code should have been deleted (well, actually there is one thing I know about-- see the first change in https://github.com/edx/edx-platform/pull/4550). Had some questions about expected behavior and a few clarifying questions. Also wondering what CSS can be removed with this PR. |
|
Do we need these styles ? |
|
BTW, Andy's branch (#4549) has the linking of subsection to outline page (scrolling to the correct location). So you don't need to worry about it in this PR. |
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.
Please remove unused after deletion modules: ModuleStoreEnum, Scope, CourseGradingModel.
|
Comments were addressed, the PR rebased and ready for final review. |
cms/static/js/base.js
Outdated
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 these 2 lines (and related) code be deleted (deleteUnit, createNewUnit). I can't think of anything that would be using them.
|
We intended that units.html be deleted. I see it is still referred to in container.html, but I believe that reference can be removed. units.html used to be used for both the subsection page and for the unit location sidebar on the container page. That sidebar has been rewritten and has its own templates now. |
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.
TriggerChangeEventOnEnter is now unused and can be removed.
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.
@cahrens TriggerChangeEventOnEnter is now unused and can be removed.
removed, as well as things related to units.html
|
👍 Looks good to merge (test failures all seem unrelated) |
Delete standalone Subsection edit page
https://openedx.atlassian.net/browse/STUD-1845
Technical details:
overview.jsandoverview_spec.coffeebase.js(or at least as much of it as possible)Implementation question:
All inplace edits were changed to mode "click the text to start editing", as I did not observe this behavior in bulk-publishing branch. Tests pass, but I would like to hear your word on it.
@auraz @polesye please take a look