Bugfix/component properties

[JohnRodney]

Description:
Adds a method to an entity to understand both 'attribute' and 'attribute.property' and returns the appropriate value in either case.
Changes proposed:

• Remove special casing of circumstances from animation.js
• Adds native method to element to understand this syntax.

[JohnRodney]

@ngokevin I opted to add the functionality to the element to understand the component attribute rather than write more special cases into animation. If you would prefer a different approach please let me know. This is in reference to #1334

[JohnRodney]

Hold off on this... even though all the tests work there are visual regressions in the system. Found when going to some examples will dig deeper. A bit strange that all animation tests work when the actual components themselves don't.

[JohnRodney]

Hmm the visual regression occurred upstream and not from this PR I will open a new issue.

[ngokevin]

Might be easier to keep this as a utility function within the animation module for now. (e.g., getComputedAttributeFor(el, attribute).

Not sure if we want to fully support dot notation because if we do it here, then we have to do it for setAttribute as well to be consistent, and it'll increase API surface area / documentation / code to maintain. If we start to see a bigger need for dot notation, then we could re-eval.

[JohnRodney]

This has been updated, but the PR is failing to update on the branch push. Looks like github status at the moment is saying some backend services unavailable. Will recheck when they report servers are running normally.

If the issue doesn't repair I will close and open a new PR.

[ngokevin]

Yeah, GitHub has been acting up last hour.

[ngokevin]

Maybe name getAttributeWithDotNotation?

[JohnRodney]

@ngokevin it gets the attribute no matter if it is dot notation or not.

[ngokevin]

Yeah, though it seems the purpose of it is to support dot notation, with fallback. Is that right?

[ngokevin]

Can we add a test for this?

[ngokevin]

Should we add a test that tests the whole animation grabbing the correct from value if using dot notation?

[JohnRodney]

Absolutely. The rest of the refactors are addressed other than changing to the function to get with dot notation because it supports both and that name might be misleading?

[ngokevin]

OK, that's fine. I'd emphasize that in the docstring more getComputedAttribute that supports dot notation to look up a component property.

[JohnRodney]

Sounds good.

[ngokevin]

Did you see my comment about adding a more integrated test?

[JohnRodney]

@ngokevin yea I wanted to see why the last refactor didn't pass the integration test even though it did local before I added another test case. Not sure if this was due to github having issues still so wanted to push the doc refactor first and ensure tests were fine before adding another.

Hmm same issue again, but not occuring on local.

[dmarcos]

I know it's a pain the ass (I'm going through this myself now) but we need to isolate the problem. @JohnRodney Does the failure go away if you comment your test out?

[JohnRodney]

@dmarcos sorry didn't get around to trying last night. I had tried to download a different version of firefox so I could get it to fail on my local, but I never got it to. Had to wrap up some other stuff on a project today will be revisiting this again in a few hours and figure it out. As I will need to use an entity generator to get a component to exist on the test.

Is it passing on all tests locally for you as well then failing on travis?

[ngokevin]

Got the tests working #1349

I think the problem is you are creating a scene outside of the setup/teardown. We have global setup methods that expect the scene to be created in a setup handler.

[ngokevin]

You can apply this diff 5bfc71a

[dmarcos]

@JohnRodney What's left to do here?

[JohnRodney]

@dmarcos Sorry with all the stuff going on and catching up with work I just haven't had the time to address this. I can close it and re-open after I finish the feature/animateColor PR. Or someone can pick up where I left off here. I know its probably bugging you to have lingering PRs.

[ngokevin]

I can volunteer to pick this up.

[JohnRodney]

@dmarcos I applied the upstream changes here.

This build seems to be failing due to docker not installing from NPM properly. This is a problem with NPM itself and not this branch. npm/npm#9418 (comment)

The problem seems to be intermittent waiting to see if this shows up somewhere else. Or just this branch I could try closing and opening a new PR. Waiting to see what ya'lls suggestions are.

[dmarcos]

@JohnRodney I have restarted the travis build. It seems you need to be an admin of the repo to do it.

[dmarcos]

You can declare a variable at the top var attributeValue and do:

attributeValue = el.getComputedAttribute(componentName);
if (attributeValue) { return attributeValue; }
return el.getComputedAttribute(componentPropName);

[JohnRodney]

@dmarcos I may be misunderstanding, but this doesn't work in the case of an attribute needing to be split. We need 3 cases here one for a default, one for a attribute split and then fallback to the native if it doesn't work.

I think I could do:

if (attributeValue) { return attributeValue[componentPropName]; }
return el.getComputedAttribute(componentPropName);

[JohnRodney]

I pushed it up with the example I showed and all tests are working. This should be good.

[dmarcos]

the code of the whole function I think would be:

function getComputedAttributeFor (el, attribute) {
  var attributeSplit = attribute.split('.'); 
  var attributeValue;
  var componentName = attributeSplit[0]; 
  var componentPropName = attributeSplit[1]; 
  // If the attribute is singular call normal getComputedAttribute function
  if (attributeSplit.length === 1) {
    return el.getComputedAttribute(componentName);
   }
  attributeValue = el.getComputedAttribute(componentName);
  if (attributeValue) { return attributeValue; }
  return el.getComputedAttribute(componentPropName);
}

[JohnRodney]

right but attributeValue would be an object such as in the case of material.color value would be material and the return you need is actually the value of color on material.

The line in question is just:

if (attributeValue) { return attributeValue; }

needing to be

if (attributeValue) { return attributeValue[componentPropName]; }

[dmarcos]

Almost there. Just one more style change.

[dmarcos]

you can do return attributeValue

[JohnRodney]

Cool those adjustments seem solid tests are still good. Ping me if anything else gonna step off for a bit for dinner ect.

[dmarcos]

Nice. Thanks for the patience :)