Pass Node ID string to run().

[iamdustan]

Thought this would be an easy one to tackle and start looking into some more of the code.

The tests with funcSetup are definitely a lot harder to follow. I had to change the check to a string from an !(node instanceof Node) since the tests createMockNode function returns a simple object.

[pvdz]

I know this is going to be an inconsistent request, but if you're going to overload the parameters, please add a jsdoc here explaining it. Yes, the function should have had a jsdoc anyways, but for whatever reason it doesn't.

[iamdustan]

Looking a bit deeper I found the run method (I’m now assuming createRenderer is private and therefore not documented). https://github.com/iamdustan/bonsai/blob/cb6ed0f511ea50b0b4510d144b357651bb8ca139/src/bootstrapper/player.js#L90

I moved the method overloading there as it seems to be a better fit (and it’s already jsdoc’d)

[pvdz]

Also, while the judges decide on this pull request (full disclosure: i'm not a judge); please also update the changelog and add a pull request for bonsai-docs (referring to this ticket in the PR there, or something, I guess) with an update for this API.

[davidaurelio]

Sorry for not providing feedback earlier. I’m not too happy with the current state of this PR for the following reasons:

1. run() is also meant to be used in DOM-less environments, especially node.js
2. I know we are not there yet, but we wanted to make all the tests runnable in node.js as well. Using global document in tests – while understanding what’s happening here – seems to be a better fit for an integration test.
3. There are renderer types (in node.js) that don’t use the DOM at all, so simply passing the string through to the renderer seems cleaner to me. The SvgRenderer could then lookup the id. It needs the DOM anyway.
4. In theory, SvgRenderer only needs to access node.ownerDocument and therefore not rely on globals, but I could live with that.

[iamdustan]

@davidaurelio great points. Introducing document to the tests felt a little dirty, especially so now. Will look into rectifying those points. I need to learn more about the renderer/runner relationship for #153 anyway.

[iamdustan]

So I was hoping that this line would work for the new Svg line in the SvgRenderer constructor, but it didn’t fly. I got it to work by making the new Svg line say new SvgRenderer.Svg but then there were other integration issues with L227 svg.root.addEventListener

Any tips?

[davidaurelio]

Hm, I’d just test whether the append() method of the fake node has been called with the root node of the movie. E.g. like this:

var targetNode = createFakeDomNode();
spyOn(targetNode, 'appendChild');
spyOn(document, 'getElementById').andReturn(targetNode);
var renderer = new SvgRenderer('thing', 1, 1);

expect(document.getElementById).toHaveBeenCalledWith('thing');
expect(targetNode.appendChild.).toHaveBeenCalledWith(renderer.svg.rootContainer);

That should do the trick

[iamdustan]

Finally updated this with your suggestions @davidaurelio. Let me know what you think. I’m done for the day. No more github spam from me ;)

[davidaurelio]

Looks good, just finish the last test (I’ve commented on it) and merge current master, then we’re good to go. Thanks for the work

[iamdustan]

And fixed. Hope you’re feeling better. Thanks for the tip!

[iamdustan]

Just pulled this down and compiled it. It doesn’t work :(

Here’s a copy of the compiled code. Hopefully will have a moment this weekend to go further into it than the test did. :/

_getSize:function(a, b, c) {
    var d;
    if(!b || !c) {
      a ? (d = a.ownerDocument.defaultView, d = d.getComputedStyle(a, null), b || (b = a.clientWidth - parseInt(d.paddingLeft) - parseInt(d.paddingRight)), c || (c = a.clientWidth - parseInt(d.paddingLeft) - parseInt(d.paddingRight))) : b = c = 0
Uncaught TypeError: Cannot read property 'defaultView' of undefined
    }
    return{width:b, height:c}

[davidaurelio]

Ok. The player module tries to get the computed width and height of the DOM element (which is a string), because either width or height weren’t passed.

Seems we need to resolve an id early. Sorry for being so nit-picky before and delegating id resolution to the svg renderer.

[davidaurelio]

This is the code: https://github.com/uxebu/bonsai/blob/master/src/bootstrapper/player.js#L127-L148

[iamdustan]

Do you think that we should resolve at the run call with: if (node && typeof node === 'string' && document) node = document.getElementById(node) or add a check to the _getSize method?

My concern with adding it to _getSize is that we will already have the check for the string and document.getElementById calls in two locations with at least two more when the other renderer’s are built.