Added block previews #1677

kahkhang · 2016-04-03T04:54:25Z

Renders Tabs, Collapsible, Image, Video RTE components as bigger blocks in the rich text editor. For Image component, the actual image is rendered, and for the Video component, a Youtube thumbnail is rendered. Addresses #909.

seanlip · 2016-04-03T05:07:30Z

core/domain/rte_component_registry_test.py

            dir_contents = self._listdir_omit_ignored(component_dir)
-            self.assertLessEqual(len(dir_contents), 5)
+            self.assertLessEqual(len(dir_contents), 6)


Should probably add some additional checks in the lines below to enforce naming conventions.

codecov-io · 2016-04-03T06:13:40Z

Current coverage is `44.00%`

Merging #1677 into develop will decrease coverage by -26.97% as of 64819c8

@@            develop   #1677   diff @@
=======================================
  Files           374     185     -189
  Stmts         37633   15058   -22575
  Branches       2405    2412       +7
  Methods           0       0         
=======================================
- Hit           26711    6626   -20085
- Partial         584     592       +8
+ Missed        10338    7840    -2498

Review entire Coverage Diff as of 64819c8

Powered by Codecov. Updated on successful CI builds.

seanlip · 2016-04-03T07:12:45Z

extensions/rich_text_components/base.py

@@ -49,6 +49,12 @@ class BaseRichTextComponent(object):
    # Whether the component requires the filesystem in some way that
    # prevents it from being used by unauthorized users.
    requires_fs = False
+    # Whether the component should be displayed as a block component
+    is_block = False


is_block_element is a better name, I think -- one says that a div is a "block element", not a "block".

seanlip · 2016-04-03T07:17:21Z

I don't think scripts/run_presubmit_checks.sh should be changed. Can the changes be reverted?

Also, @AllanYangZhou, could you take a look at this PR too (especially with regards to user-facing functionality)? Thanks!

* upstream/develop: Add Deselect All option in categories and languages selector in search bar

kahkhang · 2016-04-03T07:47:55Z

I've reverted changes to scripts/run_presubmit_checks.sh. Thanks!

seanlip · 2016-04-03T08:03:20Z

extensions/rich_text_components/base.py

+    # URL template of the component, such that when interpolated with the
+    # customization arugment dictionary, points to an image which will be
+    # rendered in the RTE preview. explorationId may be used as well.
+    preview_url_template = ''


The way this is handled is still a bit convoluted, and it would be nicer to encapsulate all the "is there an image file? if so, do this, if not do that" stuff within here. So, how about making this a @property instead?

@property def preview_url_template(self): return os.path.join(...)

Then, overwrite this for Image and Video. In addition, create preview pngs for Link and Math (that are exactly the same as the toolbar icons). I think that would be cleaner and would simplify the logic below, as well as in the frontend (there shouldn't need to be an else condition).

AllanYangZhou · 2016-04-04T19:59:52Z

@seanlip Yup! I will not be able to take a good look until this weekend due to midterms again, is that fine?

seanlip · 2016-04-04T20:00:40Z

Yep, this weekend is fine. Thanks @AllanYangZhou!

seanlip · 2016-04-09T21:39:19Z

Hi @kahkhang, @AllanYangZhou -- just following up on this!

Also, @kahkhang, did you see my additional comments?

kahkhang · 2016-04-10T20:46:45Z

Yep! Sorry for the late reply, I've been working on this on and off, but I've been stuck on creating an RTE component in say the admin page. I've tried adding the following snippet to the admin page.
<schema-based-editor schema="{type: 'html'}" local-value="test"> </schema-based-editor>

An RTE component is embedded in the admin page, but complex RTE components don't seem to appear in the toolbar. I've tried to trace the disabling mechanism to lines 906 to 914, and it seems to be the case that rteHelperService.getRichTextComponents() returns an empty array in the admin page, but loads all the rich text components in the exploration page as anticipated. Any help would be appreciated in enabling these complex RTE components from showing in the toolbar. Thanks!

seanlip · 2016-04-10T22:18:53Z

Ah, so sorry -- I took a look, and it seems like the admin page is unusual in that it has no RTE components defined. (There's no GLOBALS.RTE_COMPONENT_SPECS, so that object ends up being {} and no components are added.)

However, I actually found the answer to my question in this line of code -- i.e., we do already disable images in the RTE based on context. So that's all good!

I also had one other thought: should we use the allOrNothing mode for the call to $interpolate, and then detect if no result comes out? It strikes me that we probably want to fail really noisily in a case like that, instead of having $interpolate just use empty strings silently...

AllanYangZhou · 2016-04-10T22:29:08Z

@kahkhang @seanlip Just played around with the block previews, didn't see anything functioning incorrectly, and it looks good! Out of curiosity, are we planning to address the resize handlers on the image placeholders as part of this PR or separately?

kahkhang · 2016-04-10T23:00:35Z

@seanlip I think its a good idea to prevent unexpected errors from arising due to erroneous interpolations. I'll add that in. Thanks!

…erpolate

seanlip · 2016-04-11T01:49:31Z

Hi @AllanYangZhou, thanks for checking it out! I think the resize handlers will need be addressed separately (in a fix to #914). The issue only happens in FF, right?

Also, @kahkhang, is this ready for final review? (If so, could you please make sure you've replied to all the comments in this thread, too? Thanks!)

kahkhang · 2016-04-11T01:50:48Z

Yep! Sorry it slipped my mind. Thanks!

seanlip · 2016-04-11T02:01:45Z

core/templates/dev/head/forms/formBuilder.js

+        if (componentDefn.previewUrlTemplate) {
+          var interpolatedUrl = $interpolate(
+            componentDefn.previewUrlTemplate, false, null, true)(
+              angular.extend(customizationArgsDict,


Can we only do the extending only if explorationContextService.getPageContext() is editor/learner? (Maybe the cleanest way to do that is to add a boolean isInExplorationContext() method to the explorationContextService.)

seanlip · 2016-04-11T02:05:31Z

Thanks, @kahkhang! Just a few more comments/questions.

…ertRtetoHtml and convertHtmlToRte

seanlip · 2016-04-11T03:18:42Z

core/templates/dev/head/forms/formBuilder.js

@@ -650,7 +672,7 @@ oppia.factory('rteHelperService', [
          elt.find(
            'img.oppia-noninteractive-' + componentDefn.name
          ).replaceWith(function() {
-            var jQueryElt = $('<' + this.className + '/>');
+            var jQueryElt = $('<' + this.className.split(' ')[0] + '/>');


What's happening here? It might merit a comment.

If you're looking for the class name starting with oppia-noninteractive-*, then taking the first one is error-prone -- you don't know what might happen in the future. Instead I would suggest actually looking for the class name starting with oppia-noninteractive-*, and erroring noisily if you don't find exactly one.

seanlip · 2016-04-11T03:47:05Z

Thanks, @kahkhang. One more comment, and it's good to go!

AllanYangZhou · 2016-04-11T03:49:32Z

@seanlip Just checked: the resize issue happens in IE 11 too. I've noted this in the issue thread.

But yeah, I agree that should be separate.

seanlip · 2016-04-11T18:17:05Z

core/templates/dev/head/forms/formBuilder.js

@@ -672,7 +672,13 @@ oppia.factory('rteHelperService', [
          elt.find(
            'img.oppia-noninteractive-' + componentDefn.name
          ).replaceWith(function() {
-            var jQueryElt = $('<' + this.className.split(' ')[0] + '/>');
+            // Look for a class name starting with oppia-noninteractive-*.
+            var tagNameMatch = /(^|\s)(oppia-noninteractive-\w+)/.exec(


One gotcha here: I think \w is incorrect, and should be replaced with something that means "lowercase chars and hyphens" -- otherwise this may break in the future for RTE components with multi-word names, and it will take a while to debug. Would something like [a-z\-]+ work?

Oops missed that out, thanks!. Hmm just wondering, is there a possibility that uppercase characters or digits might be used too?

Uppercase chars -- no, not in an html tag or class name, I think. Digits -- possible but unlikely; on balance, I think we might as well include them.

seanlip · 2016-04-11T18:17:46Z

@kahkhang -- still one more comment (about your commit), sorry!

seanlip · 2016-04-11T22:06:44Z

LGTM. Thanks @kahkhang!

Added block previews

08e68b3

seanlip added the IN REVIEW - don't merge label Apr 3, 2016

seanlip assigned AllanYangZhou Apr 3, 2016

seanlip reviewed Apr 3, 2016
View reviewed changes

Addressed comments, added inline/block display CSS

8a50ea1

seanlip reviewed Apr 3, 2016
View reviewed changes

kahkhang added 2 commits April 3, 2016 03:27

Merge remote-tracking branch 'upstream/develop' into rte-preview-blocks

a4954c3

* upstream/develop: Add Deselect All option in categories and languages selector in search bar

Addressed comments

d32ea7e

seanlip reviewed Apr 3, 2016
View reviewed changes

Addressed removed comments, added allOrNothing option to call to $int…

f2e94a6

…erpolate

seanlip reviewed Apr 11, 2016
View reviewed changes

Revised code according to Sean's comments, changed unit test for conv…

b5dba15

…ertRtetoHtml and convertHtmlToRte

seanlip reviewed Apr 11, 2016
View reviewed changes

Addressed Sean's comment

987738b

seanlip reviewed Apr 11, 2016
View reviewed changes

Revised regex according to review

769de4b

seanlip added PR: LGTM and removed IN REVIEW - don't merge labels Apr 11, 2016

seanlip merged commit 19a934c into oppia:develop Apr 11, 2016

This was referenced Apr 11, 2016

Revert permissions for scripts/run_presubmit_checks.sh #1719

Closed

Revert permissions for scripts/run_presubmit_checks.sh #1720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added block previews #1677

Added block previews #1677

kahkhang commented Apr 3, 2016

seanlip Apr 3, 2016

kahkhang Apr 3, 2016

codecov-io commented Apr 3, 2016

seanlip Apr 3, 2016

kahkhang Apr 3, 2016

seanlip commented Apr 3, 2016

kahkhang commented Apr 3, 2016

seanlip Apr 3, 2016

kahkhang Apr 11, 2016

AllanYangZhou commented Apr 4, 2016

seanlip commented Apr 4, 2016

seanlip commented Apr 9, 2016

kahkhang commented Apr 10, 2016

seanlip commented Apr 10, 2016

AllanYangZhou commented Apr 10, 2016

kahkhang commented Apr 10, 2016

seanlip commented Apr 11, 2016

kahkhang commented Apr 11, 2016

seanlip Apr 11, 2016

kahkhang Apr 11, 2016

seanlip commented Apr 11, 2016

seanlip Apr 11, 2016

kahkhang Apr 11, 2016

seanlip commented Apr 11, 2016

AllanYangZhou commented Apr 11, 2016

seanlip Apr 11, 2016

kahkhang Apr 11, 2016

seanlip Apr 11, 2016

kahkhang Apr 11, 2016

seanlip commented Apr 11, 2016

seanlip commented Apr 11, 2016

Added block previews #1677

Added block previews #1677

Conversation

kahkhang commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 3, 2016

Current coverage is 44.00%

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 3, 2016

kahkhang commented Apr 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanYangZhou commented Apr 4, 2016

seanlip commented Apr 4, 2016

seanlip commented Apr 9, 2016

kahkhang commented Apr 10, 2016

seanlip commented Apr 10, 2016

AllanYangZhou commented Apr 10, 2016

kahkhang commented Apr 10, 2016

seanlip commented Apr 11, 2016

kahkhang commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 11, 2016

AllanYangZhou commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 11, 2016

seanlip commented Apr 11, 2016

Current coverage is `44.00%`