Fix missing componentName in DoughBaseComponent #219

ourmaninamsterdam · 2015-02-18T16:13:21Z

This PR accomplishes the following:

Enables a component to be instantiated using its constructor and stamped (new FooComponent()) without using componentLoader. Before initialising a component directly would return stamp data-dough-undefined-initialised="yes". It now properly sets data-dough-foo-initialised="yes". It does this by moving responsibility of setting the componentName to DoughBaseComponent. See componentname is not being set in DoughBaseComponent #218 for the original bug.
Adds componentName's to default Dough components
Adds specs for DoughBaseComponent

ourmaninamsterdam · 2015-02-19T11:35:19Z

Found this to get function name which would avoid the need to specify the componentName: https://gist.github.com/ourmaninamsterdam/53e5b87167a411ada149

benbarnett · 2015-02-19T12:25:56Z

assets/js/components/DoughBaseComponent.js

+    var warning;
+
+    if (typeof componentName !== 'undefined') {
+      if (this.$el.data('dough-component').indexOf(componentName)) {


Does this need indexOf(componentName) >= -1?

Sorry > -1

stevoland · 2015-03-05T10:27:34Z

assets/js/lib/utilities.js

+     * @returns {string} eg. tab-selector
+     */
+    convertCamelCaseToDashed: function(str) {
+      var val = str.replace(/[A-Z]/g, function(match) {


I realise you've just moved this function but it currently doesn't handle cases like `MASComponent' correctly

alexwllms · 2015-03-05T10:28:46Z

LGTM 👍

stevoland · 2015-03-05T10:29:12Z

spec/js/tests/componentLoader_spec.js

@@ -2,6 +2,13 @@ describe('componentLoader', function() {

  'use strict';

+  function convertCamelCaseToDashed(str) {


Does copying this code to the spec actually test anything? If the production code has an issue, so does the test and it isn't caught.

benbarnett · 2015-05-07T13:28:11Z

This looks great 👍

ourmaninamsterdam · 2015-05-12T10:47:27Z

@benbarnett We discussed the dependency resolution issues that could occur when including the utilities.js dependency from DoughBaseComponent.js and how we could get around these. I've done some testing and it works fine if you specify utilities.js in frontend's require_config.js.erb config with a mounted engine (pensions calc in my tests), but if you run the engine standalone and don't specify this in the engine's require_config.js.erb then Cucumber blows up. Short of including utilities.js in each of the engines JS configs, can you think of any other ways around this?

benbarnett · 2015-05-12T10:56:23Z

Hmm, yeah sadly this will only work when the engines are mounted, as they inherit the require config from frontend. We'd need to add them individually to the engines to get their tests to pass.

I suppose an alternative could just be to merge utilities into DoughBaseComponent?

benbarnett · 2015-05-12T11:18:25Z

Just discussed this in person, including the utilities module directly into the ComponentLoader.js file (via sprockets, not RequireJS) seems to have made this simpler.

#218

Will now match MASComponent.

`componentName` is set on the prototype.

@benbarnett

@benbarnett 👍

benbarnett · 2015-05-12T14:06:19Z

Travis failing is an issue separate to this and shouldn't hold up the merge. I've run the tests locally and all looks good 👍

benbarnett · 2015-05-12T14:11:47Z

assets/js/components/FieldHelpText.js

@@ -34,6 +34,8 @@ define(['jquery', 'DoughBaseComponent'], function($, DoughBaseComponent) {

  DoughBaseComponent.extend(FieldHelpText);

+	FieldHelpText.componentName = 'FieldHelpText';


Weird indentation here

…-name Fix missing componentName in DoughBaseComponent

benbarnett reviewed Feb 19, 2015
View reviewed changes

ourmaninamsterdam force-pushed the fix-missing-component-name branch 5 times, most recently from 9baa951 to 43743da Compare February 25, 2015 16:21

ourmaninamsterdam changed the title ~~[WIP][DO NOT MERGE] Check for missing component name~~ Fix missing componentName in DoughBaseComponent Feb 26, 2015

stevoland reviewed Mar 5, 2015
View reviewed changes

ourmaninamsterdam changed the title ~~Fix missing componentName in DoughBaseComponent~~ [DO NOT MERGE] - Fix missing componentName in DoughBaseComponent Mar 5, 2015

ourmaninamsterdam force-pushed the fix-missing-component-name branch from c593038 to ad1b62a Compare March 11, 2015 11:20

ourmaninamsterdam force-pushed the fix-missing-component-name branch from b9434dc to c8d7e85 Compare May 1, 2015 15:06

ourmaninamsterdam force-pushed the fix-missing-component-name branch from c8d7e85 to ced2f01 Compare May 8, 2015 09:48

ourmaninamsterdam mentioned this pull request May 8, 2015

Add componentNames to JS modules moneyadviceservice/cms#206

Merged

ourmaninamsterdam changed the title ~~[DO NOT MERGE] - Fix missing componentName in DoughBaseComponent~~ Fix missing componentName in DoughBaseComponent May 8, 2015

ourmaninamsterdam added 8 commits May 12, 2015 12:25

Add check for componentName and display warning

8f484b0

#218

Specify componentName for each Dough component

560e9c5

Tidy up spacing in Constructor

c58d1b8

Move component name converter method to utilities

290d967

Add DoughBaseComponent tests

e361fa7

Set componentName within DoughBaseComponent

e926f9a

Update DoughBaseComponent spec wording

1e2461f

Add _initialisedSuccess/Failure tests

a34bd95

ourmaninamsterdam added 10 commits May 12, 2015 12:25

Update DoughBaseComponent test wording

8f8e673

Add test for config/defaultConfig to DoughBaseComponent

6f36d0f

Update camel case convert util script

dfe0695

Will now match MASComponent.

Use camel case convert script in spec

8f1e636

Move componentName to constructor

8479406

Fix syntax inconsistencies

5f17b8c

Fix rebase conflicts

df71d84

Removes redundant componentName properties

b3cf3c4

`componentName` is set on the prototype.

Require in componentLoader utilities.js via sprockets

430a8ba

@benbarnett 👍

Bump Dough to 5.7.0

52f515c

ourmaninamsterdam force-pushed the fix-missing-component-name branch from ced2f01 to 52f515c Compare May 12, 2015 11:31

benbarnett reviewed May 12, 2015
View reviewed changes

Fix indentation

3a90fb2

ourmaninamsterdam added a commit that referenced this pull request May 13, 2015

Merge pull request #219 from moneyadviceservice/fix-missing-component…

cf4e4cb

…-name Fix missing componentName in DoughBaseComponent

ourmaninamsterdam merged commit cf4e4cb into master May 13, 2015

ourmaninamsterdam deleted the fix-missing-component-name branch May 13, 2015 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing componentName in DoughBaseComponent #219

Fix missing componentName in DoughBaseComponent #219

ourmaninamsterdam commented Feb 18, 2015

ourmaninamsterdam commented Feb 19, 2015

benbarnett Feb 19, 2015

benbarnett Feb 19, 2015

stevoland Mar 5, 2015

alexwllms commented Mar 5, 2015

stevoland Mar 5, 2015

benbarnett commented May 7, 2015

ourmaninamsterdam commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett May 12, 2015

ourmaninamsterdam May 12, 2015

		@@ -2,6 +2,13 @@ describe('componentLoader', function() {

		'use strict';

		function convertCamelCaseToDashed(str) {

		@@ -34,6 +34,8 @@ define(['jquery', 'DoughBaseComponent'], function($, DoughBaseComponent) {

		DoughBaseComponent.extend(FieldHelpText);

		FieldHelpText.componentName = 'FieldHelpText';

Fix missing componentName in DoughBaseComponent #219

Fix missing componentName in DoughBaseComponent #219

Conversation

ourmaninamsterdam commented Feb 18, 2015

ourmaninamsterdam commented Feb 19, 2015

benbarnett Feb 19, 2015

Choose a reason for hiding this comment

benbarnett Feb 19, 2015

Choose a reason for hiding this comment

stevoland Mar 5, 2015

Choose a reason for hiding this comment

alexwllms commented Mar 5, 2015

stevoland Mar 5, 2015

Choose a reason for hiding this comment

benbarnett commented May 7, 2015

ourmaninamsterdam commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett commented May 12, 2015

benbarnett May 12, 2015

Choose a reason for hiding this comment

ourmaninamsterdam May 12, 2015

Choose a reason for hiding this comment