Skip to content

Conversation

FloEdelmann
Copy link
Member

This fixes the error mentioned in #128 (comment).

There are two warnings left when running grunt closure-compiler:

>> js_src/pid_display.js:46: WARNING - Property _pid never defined on app.MessageField
>> app.MessageField.prototype.pid = function() { return this._pid; };
>>                                                           ^^^^
>> 
>> js_src/pid_display.js:190: WARNING - Property update never defined on app.MessageGroup
>>       new_div.update(field['items']);
>>               ^^^^^^

I don't know enough of their purpose to decide what to do with them.

app.MessageField.prototype.enterDocument = function() {
app.MessageField.superClass_.enterDocument.call(this);

var tt = (
Copy link
Member Author

Choose a reason for hiding this comment

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

I always thought this was an object at first glance, so I changed it to make it more clear that it's a string.

Copy link
Member

Choose a reason for hiding this comment

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

I wondered if that was a linter fix, but perhaps not looking at the blame.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

There's one more outstanding use of setHtml here

tt.setHtml('RDM Responder Test Score: ' + rating + '%');
. Would you mind fixing that too please?

'Type: ' + this._field_info['type'] + '<br>' +
'Name: ' + this._field_info['name'] + '<br>');
var tt = 'Type: ' + this._field_info['type'] + '<br>';
tt += 'Name: ' + this._field_info['name'] + '<br>';
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how good the optimiser is, but this would be slightly better if it was just one concat of strings across two lines I think, or does it then throw an error and is that what the brackets were for?

@peternewman
Copy link
Member

Hi @FloEdelmann ,

See also #210 which stops it moaning about closure code we can't control.

@peternewman
Copy link
Member

I wonder if

js_src/pid_display.js:190: WARNING - Property update never defined on app.MessageGroup
new_div.update(field['items']);

Is because of the slightly odd dependency tree?

We define MessageStructure:
https://github.com/OpenLightingProject/rdm-app/blob/master/js_src/pid_display.js#L143-L146

Then it's update method (which refers to MessageGroup's update method):
https://github.com/OpenLightingProject/rdm-app/blob/master/js_src/pid_display.js#L179-L196

Finally we define MessageGroup as a child of MessageStructure:
https://github.com/OpenLightingProject/rdm-app/blob/master/js_src/pid_display.js#L206-L210

So it's all a bit circular! It might be worth trying to move the MessageGroup stuff above the update stuff and see if that fixes the warning.

@peternewman
Copy link
Member

This one:

js_src/pid_display.js:46: WARNING - Property _pid never defined on app.MessageField
app.MessageField.prototype.pid = function() { return this._pid; };

Looks like a typo, or something that never got updated. Compare the JS Doc, to the code:
https://github.com/OpenLightingProject/rdm-app/blob/master/js_src/pid_display.js#L41-L45

In which case it's been wrong since the beginning:
50bdbdc

Anyway I suspect they should be:
app.MessageField.prototype.fieldInfo = function() { return this._field_info; };

Thoughts @nomis52 ?

@FloEdelmann
Copy link
Member Author

Moving MessageGroup above MessageStructure.update didn't help unfortunately.

'Type: ' + this._field_info['type'] + '<br>' +
'Name: ' + this._field_info['name'] + '<br>');
var tt = 'Type: ' + this._field_info['type'] + '<br>' +
'Name: ' + this._field_info['name'] + '<br>';
Copy link
Member

Choose a reason for hiding this comment

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

Can we space the 'Name: ' bit to align below Type please.

Copy link
Member

Choose a reason for hiding this comment

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

Done

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM. While it doesn't fix all the outstanding errors it certainly looks to improve things!

@peternewman peternewman merged commit 839a27d into OpenLightingProject:master Jun 27, 2018
@FloEdelmann FloEdelmann deleted the closure branch June 27, 2018 21:01
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.

3 participants