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

Emit event when component has been removed #1434

Merged
merged 3 commits into from May 7, 2016

Conversation

Projects
None yet
4 participants
@mayognaise
Contributor

mayognaise commented May 3, 2016

This PR is for this issue:
#1431

Additionally it's been attached name of component

Show outdated Hide outdated src/core/a-entity.js Outdated
@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

Can you write a quick unit test for this in a-entity.test.js? Thanks!

Member

ngokevin commented May 4, 2016

Can you write a quick unit test for this in a-entity.test.js? Thanks!

@mayognaise

This comment has been minimized.

Show comment
Hide comment
@mayognaise

mayognaise May 4, 2016

Contributor

@ngokevin hmm actually the test hasn't been passed.. the event is not called :/ do you think it's because it's already removed?

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = document.createElement('a-entity');
    var parentEl = this.el;
    el.object3D = new THREE.Mesh();
    el.addEventListener('componentremoved', function () {
      done();
    });
    el.addEventListener('loaded', function () {
      el.parentNode.removeChild(el);
    });
    parentEl.appendChild(el);
  });
Contributor

mayognaise commented May 4, 2016

@ngokevin hmm actually the test hasn't been passed.. the event is not called :/ do you think it's because it's already removed?

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = document.createElement('a-entity');
    var parentEl = this.el;
    el.object3D = new THREE.Mesh();
    el.addEventListener('componentremoved', function () {
      done();
    });
    el.addEventListener('loaded', function () {
      el.parentNode.removeChild(el);
    });
    parentEl.appendChild(el);
  });
@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

Don't need to create a Mesh, you should set a random component, use removeAttribute on it, and check the event detail has the component name. You can use the existing entity created in the setup this.el

Member

ngokevin commented May 4, 2016

Don't need to create a Mesh, you should set a random component, use removeAttribute on it, and check the event detail has the component name. You can use the existing entity created in the setup this.el

@mayognaise

This comment has been minimized.

Show comment
Hide comment
@mayognaise

mayognaise May 4, 2016

Contributor

@ngokevin using entityFactory();?
and why using removeAttribute ?
im afraid to ask if you give me an example... 😨 sorry im not good at this..

Contributor

mayognaise commented May 4, 2016

@ngokevin using entityFactory();?
and why using removeAttribute ?
im afraid to ask if you give me an example... 😨 sorry im not good at this..

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

In the test suite, there should already be a setup that calls entityFactory for you.

removeAttribute detaches components, which is what you're testing here. removeChild would detach all components I guess, but it's simpler to test with removeAttribute.

Member

ngokevin commented May 4, 2016

In the test suite, there should already be a setup that calls entityFactory for you.

removeAttribute detaches components, which is what you're testing here. removeChild would detach all components I guess, but it's simpler to test with removeAttribute.

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

ex:

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = this.el;
    el.setAttribute('geometry');
    el.addEventListener('componentremoved', function (evt) {
      assert.equal(evt.detail.name, 'geometry');
      done();
    });
    el.removeAttribute('geometry');
  });
Member

ngokevin commented May 4, 2016

ex:

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = this.el;
    el.setAttribute('geometry');
    el.addEventListener('componentremoved', function (evt) {
      assert.equal(evt.detail.name, 'geometry');
      done();
    });
    el.removeAttribute('geometry');
  });
@mayognaise

This comment has been minimized.

Show comment
Hide comment
@mayognaise

mayognaise May 4, 2016

Contributor

@ngokevin ohh i see
so this doesn’t mean when the a-entity has been removed from dom, correct?

Contributor

mayognaise commented May 4, 2016

@ngokevin ohh i see
so this doesn’t mean when the a-entity has been removed from dom, correct?

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

yes just testing component detach

Member

ngokevin commented May 4, 2016

yes just testing component detach

@mayognaise

This comment has been minimized.

Show comment
Hide comment
@mayognaise

mayognaise May 4, 2016

Contributor

@ngokevin i see. componentremoved hasn't been fired.. 😭
I may be doing a wrong PR, but what I expected is to know when a-entity has removed from shader side, like component has remove function.

Contributor

mayognaise commented May 4, 2016

@ngokevin i see. componentremoved hasn't been fired.. 😭
I may be doing a wrong PR, but what I expected is to know when a-entity has removed from shader side, like component has remove function.

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

el.setAttribute('geometry', ''); fixed this line

Member

ngokevin commented May 4, 2016

el.setAttribute('geometry', ''); fixed this line

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

you might want something for detachedCallback, but this PR is ok too

Member

ngokevin commented May 4, 2016

you might want something for detachedCallback, but this PR is ok too

@ngokevin

This comment has been minimized.

Show comment
Hide comment
@ngokevin

ngokevin May 4, 2016

Member

I'll check this PR out later and try to add a proper test.

Member

ngokevin commented May 4, 2016

I'll check this PR out later and try to add a proper test.

@mayognaise

This comment has been minimized.

Show comment
Hide comment
@mayognaise

mayognaise May 4, 2016

Contributor

@ngokevin the test has been passed 🎉

Contributor

mayognaise commented May 4, 2016

@ngokevin the test has been passed 🎉

@@ -312,6 +312,7 @@ var proto = Object.create(ANode.prototype, {
pauseComponent(component, this.sceneEl);
component.remove();
delete this.components[name];
this.emit('componentremoved', { name: name });

This comment has been minimized.

@dmarcos

dmarcos May 4, 2016

Collaborator

should we pass the entity as well?

@dmarcos

dmarcos May 4, 2016

Collaborator

should we pass the entity as well?

This comment has been minimized.

@dmarcos

dmarcos May 4, 2016

Collaborator

not sure if we should pass it. it would be done this way:
this.emit('componentremoved', { el: this, name: name });

@dmarcos

dmarcos May 4, 2016

Collaborator

not sure if we should pass it. it would be done this way:
this.emit('componentremoved', { el: this, name: name });

This comment has been minimized.

@mayognaise

mayognaise May 4, 2016

Contributor

@dmarcos sure let me add el

@mayognaise

mayognaise May 4, 2016

Contributor

@dmarcos sure let me add el

This comment has been minimized.

@donmccurdy

donmccurdy May 4, 2016

Member

would it already be available as event.target? that seems like enough if so, otherwise maybe helpful to add it.

@donmccurdy

donmccurdy May 4, 2016

Member

would it already be available as event.target? that seems like enough if so, otherwise maybe helpful to add it.

This comment has been minimized.

@mayognaise

mayognaise May 4, 2016

Contributor

ah true, let me check

@mayognaise

mayognaise May 4, 2016

Contributor

ah true, let me check

This comment has been minimized.

@mayognaise

mayognaise May 4, 2016

Contributor

This has passed. so maybe dont we need to add it?

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = this.el;
    el.setAttribute('geometry', 'primitive:plane');
    el.addEventListener('componentremoved', function (event) {
      assert.equal(event.target, el);
      assert.equal(event.detail.name, 'geometry');
      done();
    });
    el.removeAttribute('geometry');
  });
@mayognaise

mayognaise May 4, 2016

Contributor

This has passed. so maybe dont we need to add it?

  test('emits `componentremoved` event when element itself has been removed', function (done) {
    var el = this.el;
    el.setAttribute('geometry', 'primitive:plane');
    el.addEventListener('componentremoved', function (event) {
      assert.equal(event.target, el);
      assert.equal(event.detail.name, 'geometry');
      done();
    });
    el.removeAttribute('geometry');
  });
@dmarcos

This comment has been minimized.

Show comment
Hide comment
@dmarcos

dmarcos May 7, 2016

Collaborator

Nice work. @mayognaise thanks!

Collaborator

dmarcos commented May 7, 2016

Nice work. @mayognaise thanks!

@dmarcos dmarcos merged commit dd8b478 into aframevr:master May 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment