Skip to content
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

Single property components have to return the default value when the … #1631

Merged
merged 1 commit into from
Jul 18, 2016

Conversation

dmarcos
Copy link
Member

@dmarcos dmarcos commented Jul 14, 2016

No description provided.

@ngokevin
Copy link
Member

Going to assume work-in-progress based on discussion yesterday.

@dmarcos
Copy link
Member Author

dmarcos commented Jul 15, 2016

The code is simpler now and works as expected but It introduces a wrinkle in the consistency of the system that I don't know how to do differently. Look at the following entity with a single property component called dummy with true as default value.

  <a-entity position="" rotation="" scale="" visible="" dummy=""></a-entity>

position, rotation, scale and visible are default components so they're not really defined on the entity (a mixin would take precedence) On the contrary dummy is not a default component and hence really defined on the entity. When calling getAttribute default and non default components will produce different values:
entityEl.getAttribute('dummy') === true
entityEl.getAttribute('position') === undefined
entityEl.getComputedAttribute('position') === { x: 0, y: 0, z: 0 }
Does it make sense?

@dmarcos dmarcos force-pushed the emptyAttribute branch 2 times, most recently from 22a6a52 to 4f7b5fd Compare July 16, 2016 16:43
@dmarcos
Copy link
Member Author

dmarcos commented Jul 16, 2016

After discussion with @ngokevin this is how the API behaves with the latest changes. Given a single property component dummywith a default value true and the following entity:

<a-entity dummy></a-entity>

These are the values returned for the components

el.getAttribute('dummy') === true;
el.getAttribute('position') === null;
el.getComputedAttribute('position') === { x: 0, y: 0, z: 0 };

The only inconsistency left it's when opening the DOM inspector the entity above will appear as:

<a-entity position="" rotation="" scale="" scale="" visible="" dummy></a-entity>

When calling getAttribute on those components you would expect to get the default values as it happens with dummy. Default components will return null instead because they're not really defined on the entity element despite of being shown as attributes. To retrieve the value of the default components one has to call getComputedAttribute. We can live with this small inconsistency for now and revisit the getAttribute / getComputedAttribute API for next version.

@dmarcos dmarcos force-pushed the emptyAttribute branch 2 times, most recently from 4244c79 to aac6a75 Compare July 18, 2016 18:48
return;
}
this.attrValue = value !== undefined ? extendProperties({}, value, isSinglePropSchema) : this.attrValue;
var attrValue = this.attrValue = this.parseAttrValueForCache(value);
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to assign to this.attrValue here since it gets replaced.

@ngokevin
Copy link
Member

r+

@dmarcos dmarcos merged commit 365829f into aframevr:master Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants