Fix part of #2575: RTCs refactor second milestone #4017

vojtechjelinek · 2017-10-23T18:45:19Z

Moved information about RTCs from Component.py files to specs.js file.
Loading specs.js directly from frontend and backend.
specs.js file validity checked in tests.

codecov-io · 2017-10-23T20:22:06Z

Codecov Report

Merging #4017 into develop will decrease coverage by 0.02%.
The diff coverage is 81.81%.

@@             Coverage Diff             @@
##           develop    #4017      +/-   ##
===========================================
- Coverage    45.87%   45.85%   -0.03%     
===========================================
  Files          300      300              
  Lines        21169    21196      +27     
  Branches      3327     3329       +2     
===========================================
+ Hits          9712     9719       +7     
- Misses       11457    11477      +20

Impacted Files	Coverage Δ
...dev/head/services/SpeechSynthesisChunkerService.js	`22.53% <ø> (+1.4%)`	⬆️
core/templates/dev/head/app.js	`44.69% <81.81%> (+0.02%)`	⬆️
...ploration_player/AudioTranslationManagerService.js	`44.73% <0%> (-1.54%)`	⬇️
...ev/head/pages/exploration_editor/EditorServices.js	`34.37% <0%> (-0.31%)`	⬇️
core/templates/dev/head/pages/splash/Splash.js	`4.76% <0%> (-0.24%)`	⬇️
...head/pages/exploration_editor/ExplorationEditor.js	`5.03% <0%> (-0.04%)`	⬇️
...ges/exploration_editor/settings_tab/SettingsTab.js	`0.64% <0%> (-0.01%)`	⬇️
...ev/head/pages/exploration_player/PlayerServices.js	`1.04% <0%> (ø)`	⬆️
...tes/dev/head/services/explorationContextService.js	`70.21% <0%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57da863...9e65481. Read the comment docs.

seanlip

Thanks, @vojtechjelinek! I think this looks great. I just have a few minor comments.

seanlip · 2017-10-24T06:16:10Z

assets/rich_text_components_specs.js

@@ -0,0 +1,231 @@
+/* eslint-disable */
+/* Don't modify anything outside the {} brackets.
+ * Insides of the {} brackets should be formatted as a JSON object.


The contents of the {} brackets should ...

seanlip · 2017-10-24T06:16:19Z

assets/rich_text_components_specs.js

+ * JSON rules:
+ * 1. All keys and string values must be enclosed in double quotes.
+ * 2. Each key/value pair should be on a new line.
+ * 3. All values and keys must be constant, you can't use any Javascript


seanlip · 2017-10-24T06:16:32Z

assets/rich_text_components_specs.js

+        }
+      },
+      "default_value": "You have opened the collapsible block."
+      }]


Deindent by 2.

seanlip · 2017-10-24T06:19:31Z

core/domain/rte_component_registry_test.py


            # Check that the component directory exists.
            component_dir = os.path.join(
-                feconf.RTE_EXTENSIONS_DIR, component_id)
+                feconf.RTE_EXTENSIONS_DIR, component_name)
            self.assertTrue(os.path.isdir(component_dir))

            # In this directory there should be a config .py file, an
            # html file, a JS file, an icon .png file and a protractor.js file,


Update this sentence?

seanlip · 2017-10-24T06:20:14Z

core/domain/rte_component_registry_test.py

            # Check that the component id is valid.
-            self.assertTrue(self._is_camel_cased(component_id))
+            self.assertTrue(self._is_camel_cased(component_name))


Why don't we keep it as component_id? ID implies uniqueness.

Perhaps we should rename backend_name to backend_id as well?

Yes, it should be id.

For the backend_name I was trying to be consistent with frontend_name.

OK. I'm fine with changing both to id, if you like, but up to you.

seanlip · 2017-10-24T06:22:47Z

core/templates/dev/head/app.js

@@ -72,8 +72,7 @@ oppia.constant('EVENT_ACTIVE_CARD_CHANGED', 'activeCardChanged');
 // The conditioning on window.GLOBALS.RTE_COMPONENT_SPECS is because, in the
 // Karma tests, this value is undefined.
 oppia.constant(
-  'RTE_COMPONENT_SPECS',
-  window.GLOBALS.RTE_COMPONENT_SPECS ? window.GLOBALS.RTE_COMPONENT_SPECS : {});
+  'RTE_COMPONENT_SPECS', richTextComponents ? richTextComponents : {});


Actually if we specify this in karma-globals then can we get rid of the ternary operator conditioning?

seanlip · 2017-10-24T06:23:02Z

core/templates/dev/head/app.js

@@ -517,7 +517,9 @@ oppia.factory('rteHelperService', [
      },
      createToolbarIcon: function(componentDefn) {
        var el = $('<img/>');
-        el.attr('src', componentDefn.iconDataUrl);
+        el.attr('src',


Break after '(' if contents of (...) span multiple lines.

seanlip · 2017-10-24T06:23:57Z

core/templates/dev/head/app.js

-        var interpolatedUrl = $interpolate(
-          componentDefn.previewUrlTemplate, false, null, true)(
-          customizationArgsDict);
+        var componentPreviewUrl = componentDefn.previewUrlTemplate;


Shouldn't this be called componentPreviewUrlTemplate? A URL template != URL.

seanlip

Oops, sorry -- noticed a few more things!

seanlip · 2017-10-25T07:05:58Z

core/domain/rte_component_registry.py

-            if 'BaseRichTextComponent' in ancestor_names:
-                cls._rte_components[clazz.__name__] = clazz()
+        with open(feconf.RTE_EXTENSIONS_SPECS_PATH, 'r') as f:
+            cls._rte_components = constants.parse_json_from_js(f)

    @classmethod
    def get_all_rte_components(cls):
        """Get a list of instances of all custom RTE components."""
        if len(cls._rte_components) == 0:


If I'm reading correctly, the data looks like a dict, but the docstring here and the use of len() indicate a list. Is this a mistake? If so let's fix this to say something like "if not cls._rte_components" and change the docstring to "Get a dict mapping RTE component IDs to their definitions." or similar.

seanlip · 2017-10-25T07:08:47Z

core/templates/dev/head/app.js

-          componentDefn.previewUrlTemplate, false, null, true)(
-          customizationArgsDict);
+        var componentPreviewUrlTemplate = componentDefn.previewUrlTemplate;
+        if (componentDefn.previewUrlTemplate.startsWith(


Try indexOf() === 0 here; startsWith is less browser-compatible. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith

seanlip · 2017-10-25T07:10:56Z

assets/rich_text_components_specs.js

@@ -0,0 +1,231 @@
+/* eslint-disable */


Btw, I just realized, if this is going in assets, should we pull protractor.js out of extensions/rich_text_components as well and move it to somewhere in core/tests? Then delete extensions/rich_text_components altogether?

On the other hand, if extensions/rich_text_components is staying, then shouldn't this file go in it (like rule_templates.json goes in extensions/interactions)?

We need extensions/rich_text_components because there are other files we use. I would have to create new rule for this file in app.yaml in order to serve it from extensions/rich_text_components

Hm. How does extensions/interactions/rule_templates.json do it?

That file is not directly loaded from the frontend.

I see, OK. In that case, let's keep it in assets/ and put a README.md file in extensions/rich_text_components that points to it.

seanlip · 2017-10-25T23:38:24Z

extensions/rich_text_components/README.md

@@ -0,0 +1,6 @@
+Steps to successfully add new Rich Text Component:
+1. Create folder in _extensions/rich_text_components_ with all the files
+2. Create new record in JSON in _asseets/rich_text_components_specs.js_


NB: assets is misspelled.

seanlip · 2017-10-25T23:39:24Z

extensions/rich_text_components/README.md

+2. Create new record in JSON in _asseets/rich_text_components_specs.js_
+3. Add script-imports into _core/templates/dev/head/components/rich_text_components.html_
+
+[More in Wiki](https://github.com/oppia/oppia/wiki/Rich-Text-Editor-%28RTE%29-Overview)


I suggest linking more specifically, to this: https://github.com/oppia/oppia/wiki/Rich-Text-Editor-%28RTE%29-Overview#rich-text-components

Please also update the wiki page. Thanks!

seanlip

Two small issues but otherwise LGTM; please feel free to submit after addressing them. Thanks!

RTCS refactor second milestone

66c0747

vojtechjelinek assigned seanlip Oct 23, 2017

vojtechjelinek requested a review from seanlip October 23, 2017 18:45

fix backend tests

8f48561

seanlip requested changes Oct 24, 2017

View reviewed changes

resolved comments

5255523

seanlip reviewed Oct 25, 2017

View reviewed changes

resolved comments

2390b76

seanlip reviewed Oct 25, 2017

View reviewed changes

seanlip approved these changes Oct 25, 2017

View reviewed changes

resolved comments

9e65481

seanlip merged commit 65745a1 into oppia:develop Oct 26, 2017

vojtechjelinek mentioned this pull request Nov 5, 2017

Create HTML file for RTE components in the build process. #3885

Closed

vojtechjelinek deleted the RTCs-refactor2 branch June 22, 2018 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #2575: RTCs refactor second milestone #4017

Fix part of #2575: RTCs refactor second milestone #4017

vojtechjelinek commented Oct 23, 2017

codecov-io commented Oct 23, 2017 •

edited

Loading

seanlip left a comment

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip Oct 24, 2017

vojtechjelinek Oct 24, 2017

seanlip left a comment

seanlip Oct 25, 2017

vojtechjelinek Oct 25, 2017

seanlip Oct 25, 2017

vojtechjelinek Oct 25, 2017

seanlip Oct 25, 2017

vojtechjelinek Oct 25, 2017

seanlip Oct 25, 2017

vojtechjelinek Oct 25, 2017 •

edited

Loading

seanlip Oct 25, 2017

vojtechjelinek Oct 25, 2017

seanlip Oct 25, 2017

seanlip Oct 25, 2017

seanlip left a comment

Fix part of #2575: RTCs refactor second milestone #4017

Fix part of #2575: RTCs refactor second milestone #4017

Conversation

vojtechjelinek commented Oct 23, 2017

codecov-io commented Oct 23, 2017 • edited Loading

Codecov Report

seanlip left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

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

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

vojtechjelinek Oct 25, 2017 • edited Loading

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

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 23, 2017 •

edited

Loading

vojtechjelinek Oct 25, 2017 •

edited

Loading