-
Notifications
You must be signed in to change notification settings - Fork 7
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
Studio support for creating and editing libraries (PR 6046) #23
Conversation
SOL-1, SOL-2, SOL-3
@@ -289,10 +289,42 @@ | |||
|
|||
// ==================== | |||
|
|||
// Course/Library tabs | |||
#course-index-tabs { |
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.
We don't use id based selectors, and are also removing all pure element selectors with an upcoming sass cleanup, so it would be good to replace this and any other id/element based selectors from this diff.
Thanks @antoviaque for creating this pull request. @frrrances will be reviewing this from a FED standpoint, and it would also be great to have a review from @cptvitamin for input from an a11y perspective. This PR has already merged to master due to dependencies on a number of other follow-on pull requests. The hope is that with this PR we can still get a chance to track and eventually resolve any FED and A11Y concerns/improvements. Thanks all! |
'ErrMsg': _( | ||
'There is already a library defined with the same ' | ||
'organization and library code. Please ' | ||
'change either organization or library code to be unique.' |
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.
Users are not likely to be changing their organization name. Suggest changing the 2nd sentence to: "Change your library code so that it is unique within your organization."
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.
The error message was copied from the courses (https://github.com/edx/edx-platform/blob/16809ddf1c32b04dc892e177b486bc63344e4824/cms/djangoapps/contentstore/views/course.py#L576-L579). If we change it for the libraries it would probably make sense to change it for courses, too.
A few small changes. Once those are addressed, 👍 (even though it's already merged...) |
@marcotuts @frrrances @catong FYI the comments made in the current PR will be addressed in https://github.com/edx/edx-platform/pull/6388 (SOL-80) -- most of your comments above are already implemented there. Closing the current PR - if you have any additional comment, you can post them in https://github.com/edx/edx-platform/pull/6388 , we'll address them as part of the review there. |
@catong @marcotuts @frrrances Please take a look at https://github.com/edx/edx-platform/pull/6388 (SOL-80), most of your comments have been addressed there. |
Duplicates https://github.com/edx/edx-platform/pull/6046 to allow Marco to review without spamming the reviewers on the original PR.