Skip to content

Conversation

@antgonzales
Copy link
Contributor

The current tests don't setup the components to test these attributes against expectations.

@antgonzales
Copy link
Contributor Author

@robdodson any chance I can get feedback on this pull request?

Copy link
Contributor

@robdodson robdodson left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Commented.

let root = prep("<comp-with-props>")
scope.$digest()
let wc = root.querySelector('#wc')
let data = wc.arr || wc.getAttribute('arr')
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be calling getAttribute here. For arrays and objects, the test is trying to assert that they are always passed as JS properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

let root = prep("<comp-with-props>")
scope.$digest()
let wc = root.querySelector('#wc')
let data = wc.obj || wc.getAttribute('obj')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

@robdodson robdodson left a comment

Choose a reason for hiding this comment

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

oops. meant to click 'request changes'.

@antgonzales antgonzales force-pushed the fix/advanced-angularjs-tests branch from fad5f9a to 8d51383 Compare January 18, 2018 18:40
@antgonzales
Copy link
Contributor Author

@robdodson thanks for the feedback! Fixed.

@robdodson
Copy link
Contributor

Just started reviewing :)

:octocat: Sent from GH.

@robdodson
Copy link
Contributor

Looks good. Since I'm totally not an AngularJS expert, can you explain a bit about what this change actually does? I noticed the test still fails (which is what I expected because AngularJS isn't setting properties here). But I was just curious what all the setup work does.

@antgonzales
Copy link
Contributor Author

Happy to explain! What I'm doing is compiling isolated instances of the angular components that are supposed to pass more complicated data to the child custom elements inside. Since wc (the custom element inside the compiled angular components) was not actually defined, it was comparing an undefined variable against the expectations.

Here's the angular component that wasn't getting rendered before:
https://github.com/webcomponents/custom-elements-everywhere/blob/master/libraries/angularjs/src/components.js#L56

It now correctly compiles the angular component with properties/array and tests if the custom element inside is receiving them.

@robdodson robdodson merged commit 82f002c into webcomponents:master Jan 19, 2018
@robdodson
Copy link
Contributor

Cool, thanks for the PR!

@antgonzales antgonzales deleted the fix/advanced-angularjs-tests branch January 20, 2018 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants