New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows defining multiple compoenents of a specific type on a single e… #1596

Merged
merged 1 commit into from Jul 1, 2016

Conversation

Projects
None yet
2 participants
@dmarcos
Copy link
Collaborator

dmarcos commented Jun 30, 2016

No description provided.

@@ -290,19 +294,23 @@ var proto = Object.create(ANode.prototype, {

// Check if component already initialized.
if (name in this.components) { return; }
component = this.components[name] = new components[name].Component(this, data);
if (id && !components[name].multiple) {
throw new Error('Trying to initialize multiple components of type ' + name +

This comment has been minimized.

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

and put backticks around name

@@ -278,7 +278,11 @@ var proto = Object.create(ANode.prototype, {
initComponent: {
value: function (name, data, isDependency) {
var component;
var componentInfo = name.split('__');
var id = componentInfo[1];

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

componentId or instanceId maybe to diff from HTML ID

}

// All sound values set. Load in `src`.
if (srcChanged) { sound.load(data.src); }
},

/**
* update listener attached to the user defined on event.

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

nit: could cap this sentence

assert.notOk(el.components.geometry_1);
});

test('initializes components with id if it opts in to multiple', function () {

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

nit: s/in to/into and s/it/the component

@@ -381,6 +393,16 @@ suite('a-entity', function () {
el.setAttribute('class', 'pied piper');
assert.equal(el.getAttribute('class'), 'pied piper');
});

test('retrieves data from a multiple component', function () {

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

could probably test getComputedAttribute as well

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

we should also test multi-component detach with removeAttribute

@@ -75,4 +71,19 @@ suite('sound', function () {
assert.ok(sound.pause.called);
});
});

suite('play', function () {

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

could test the sound component if used with multi, that the object3dname is set correctly

This comment has been minimized.

@dmarcos

dmarcos Jun 30, 2016

Author Collaborator

object3Dname uses now this.attrName so multi or not it's the same code path

This comment has been minimized.

@ngokevin

ngokevin Jun 30, 2016

Member

maybe you can test that this.attrName is correctly being used with setObject3D

@ngokevin

This comment has been minimized.

Copy link
Member

ngokevin commented Jun 30, 2016

I found removeAttribute doesn't detach the component.

@ngokevin ngokevin added needs changes and removed needs review labels Jun 30, 2016

@dmarcos dmarcos force-pushed the dmarcos:multipleComponents branch 2 times, most recently from f9eb1cf to fd97cfd Jul 1, 2016

@dmarcos dmarcos added needs review and removed needs changes labels Jul 1, 2016

var component;
var isComponentDefined = checkComponentDefined(this, name) || data !== undefined;
var componentInfo = attrName.split('__');

This comment has been minimized.

@ngokevin

ngokevin Jul 1, 2016

Member

Probably good to make a constant for this.

MULTI_DELIMITER = '__';

if (componentId && !components[componentName].multiple) {
throw new Error('Trying to initialize multiple ' +
'components of type `' + componentName +
'` There can only be one component of this type per entity.');

This comment has been minimized.

@ngokevin

ngokevin Jul 1, 2016

Member

period before There

@ngokevin

This comment has been minimized.

Copy link
Member

ngokevin commented Jul 1, 2016

Add test for getComputedAttribute for multi

r+wc

@dmarcos dmarcos force-pushed the dmarcos:multipleComponents branch from fd97cfd to 6bdce07 Jul 1, 2016

@dmarcos dmarcos merged commit 380e4d2 into aframevr:master Jul 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ngokevin

This comment has been minimized.

Copy link
Member

ngokevin commented Jul 1, 2016

nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment