Skip to content
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

Adds Annotatable XBlock extracting from edx-platform repo #5

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

irtazaakram
Copy link
Member

@irtazaakram irtazaakram commented Jul 24, 2024

Adds annotatable xblock extracting from edx-platform repo

Test PR

Make a test PR on edx-platform like this to run test cases on under development PR.

File references:

  • xblocks_contrib/annotatable/annotatable.py has been extracted from
  • xblocks_contrib/annotatable/templates/annotatable.html has been extracted from
  • xblocks_contrib/annotatable/templates/annotatable_editor.html has been extracted from
  • xblocks_contrib/annotatable/static/js/src/annotatable.js has been extracted from
  • xblocks_contrib/annotatable/static/js/src/annotatable_editor.js has been extracted from
  • xblocks_contrib/annotatable/static/css/annotatable.css has been extracted from
  • xblocks_contrib/annotatable/static/css/annotatable_editor.css has been extracted from
  • xblocks_contrib/annotatable/tests/test_annotatable.py has been extracted from

Acceptance Criteria:

  • Extracted XBlock user experience should behave normal if required global css variables donesn't exist
  • Extracted XBlock funcationality should work fine after turning its relevant Django Settings Flag
  • In existing built in xblock implementation of edx-platform, update the status or data of the XBlocks. Then change the Django Settings flag and verify after shifting to extracted one, XBlocks are maintaining their required states (including XBlock completion status).
  • Also test the vice versa of above step, change XBlock from Extracted one to Built in implementation to check fallback case testings.
  • i18n is supported in the XBlock
  • Create a course, add multiple XBlocks in the course. Keep different XBlocks in diversity of states (done/un-done etc). Export the course then validate the XBlock states after importing it again.
  • Make a test PR for the extracted PR like this and fix the test cases if required.
  • Write further unit test cases for good code coverage

Base automatically changed from core-xblocks to main October 7, 2024 13:59
@irtazaakram irtazaakram marked this pull request as ready for review October 21, 2024 06:50
@irtazaakram irtazaakram requested a review from farhan October 22, 2024 07:54
Generate initial i18n with dummy method.
"""
return translation.gettext_noop("Dummy")
def resource_string(self, path):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use xblock.utils.resources module to load the resources

var annotationsHidden = false;
var instructionsHidden = false;

var el = element;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not using it

var toggleAnnotationsSelector = '.annotatable-toggle-annotations';
var toggleInstructionsSelector = '.annotatable-toggle-instructions';
var instructionsSelector = '.annotatable-instructions';
var sectionSelector = '.annotatable-section';
Copy link
Contributor

Choose a reason for hiding this comment

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

Some selectors are un-used, shouldn't we remove them to keep code clean.

var $el = $(element);

if (_debug) {
console.log('loaded Annotatable');
Copy link
Contributor

Choose a reason for hiding this comment

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

'Annotatable XBlock Loaded'
sounds better
We should put it at end of init() function

}

function toggleAnnotations() {
annotationsHidden = !annotationsHidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra code line

        var hide = !annotationsHidden;

/*
Update the position object (used by qTip2 to show the tip after the move event)
*/
$.extend(position, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some functions are not returning value like in BuiltIn XBlock code, shouldn't we follow the same?

Comment on lines +249 to +250
instructionsHidden = !instructionsHidden;
var hide = instructionsHidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge the lines

@@ -1,9 +1,190 @@
/* CSS for AnnotatableBlock */
@import url("https://fonts.googleapis.com/css?family=Open+Sans:300,400,400i,600,700");
Copy link
Contributor

Choose a reason for hiding this comment

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

screencapture-text-compare-2025-02-18-11_37_06

Some values are hard coded

instructions = xmltree.find("instructions")
if instructions is not None:
instructions.tag = "div"
xmltree.remove(instructions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2025-02-18 at 4 33 11 PM

Instruction is not removing from the xml tree as its appearing 2 times within XBlock.
We should fix it (though it exist in the Built In Xblock as well), seems a bug
We should also write a test case of this deletion



class TestAnnotatableBlock(TestCase):
"""Tests for AnnotatableBlock"""
class MockRuntime(Runtime):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the TestRuntimes exist in the xblock.test api

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.

2 participants