Skip to content

Studio support for creating and editing libraries (PR 6046) - #23

Closed
antoviaque wants to merge 1 commit into
master-20141210from
edx/content-libraries
Closed

Studio support for creating and editing libraries (PR 6046)#23
antoviaque wants to merge 1 commit into
master-20141210from
edx/content-libraries

Conversation

@antoviaque

Copy link
Copy Markdown
Member

Duplicates https://github.com/edx/edx-platform/pull/6046 to allow Marco to review without spamming the reviewers on the original PR.

Copy link
Copy Markdown

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.

@marcotuts

Copy link
Copy Markdown

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!

Copy link
Copy Markdown

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."

Copy link
Copy Markdown
Member

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.

@frrrances

Copy link
Copy Markdown

A few small changes. Once those are addressed, 👍 (even though it's already merged...)

@antoviaque

Copy link
Copy Markdown
Member Author

@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.

@mtyaka

mtyaka commented Jan 5, 2015

Copy link
Copy Markdown
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants