Skip to content

Conversation

@lduarte1991
Copy link
Contributor

Background: A few comments in #2515 stated that I should move the javascript and css code from static and into the xmodule/js and xmodule/css folders. This would allow for the js and css to be called upon only if and when the module is created.

Goal of this PR: There should be no new functionality added, but this PR will make sure that the javascript and css for the annotation tool are only pulled in when the module is used. Moving these files caused a few UI issues which have also been patched within the code.

CMS Updates: You should now be able to create annotations within the CMS (this is not really used as of yet and the annotations cannot be viewed by students, only by instructors visiting Studio) which will not appear in LMS, but only when you visit the Studio end for the unit/module. The functionality should be there fully, leaving annotations, diacritic marks, highlighting, tagging. The CMS has its own table below the text that can be used as seen in the LMS.

LMS Updates: The biggest change visually is that the skin used for the TinyMCE editor is now the one used by Studio and not the one that was previously loaded onto the code. The rest should be the same as it was in a previous iteration.

Testing notes: Probably hardest PR to test. Visually everything should look the same as before (except the TinyMCE Editor) and it should have the same functionality. I'd suggest making a few annotations, editing them, deleting them, tagging them, adding diacritic marks, replying to one. Overall most of the "testing" should just be that the code is in the correct place.

@mhoeber
Copy link
Contributor

mhoeber commented May 1, 2014

@srpearce DOC-95 in backlog

Stephen Sanchez and others added 3 commits May 14, 2014 09:56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write docstring according to edx doc style: https://github.com/edx/edx-platform/wiki/Python-Guidelines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new line at the end of file. Please add it.

@auraz
Copy link
Contributor

auraz commented May 26, 2014

Please look at failing tests. Also please provide python and js test coverage and ensure coverage >= 95%.

How can I test it manually? What steps should I do? Is there any documentation on that so I can look at?

Have you consider writing acceptance tests for this feature?

Thank you.

@auraz
Copy link
Contributor

auraz commented May 26, 2014

Please add changelog.

@flowerhack
Copy link
Contributor

@lduarte1991 , @sarina and I are going to start reviewing this PR this week—can you please rebase against master before we do that? Thanks!

@lduarte1991
Copy link
Contributor Author

Absolutely will do it soon.

@sarina
Copy link
Contributor

sarina commented Jun 2, 2014

@lduarte1991 since #3480 has been merged, please rebase this PR onto master.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have css file inside js directory?

@sarina
Copy link
Contributor

sarina commented Jun 2, 2014

@polesye let's avoid new comments until this PR is nicely rebased; it contains a lot of work from #3480.

@polesye
Copy link
Contributor

polesye commented Jun 2, 2014

@sarina okay.

@lduarte1991
Copy link
Contributor Author

Yes, sorry! Working on it right now.

@lduarte1991
Copy link
Contributor Author

Question: All these pull requests were based on each other. Because we had to move PR #3831 up to after #3466 and so many different squashing and rebasing has happened in all of the PRs in between would you guys be against me closing this PR and start off from a clean branch off of master?

@polesye
Copy link
Contributor

polesye commented Jun 2, 2014

Good for me.

@sarina
Copy link
Contributor

sarina commented Jun 2, 2014

@lduarte1991 yes, that would actually be a lot easier.

@sarina
Copy link
Contributor

sarina commented Jun 2, 2014

Please ping me and @polesye in the new PR.

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

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants