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

Fix bad behavior reference in shop-cart-modal #105

Closed
wants to merge 1 commit into from

Conversation

FredKSchott
Copy link
Contributor

The new analyzer caught a bug here, where shop-cart-modal is referencing IronOverlayBehaviorImpl instead of IronOverlayBehavior. See https://github.com/PolymerElements/iron-overlay-behavior/blob/master/iron-overlay-behavior.html for their definitions.

/cc @keanulee I think? Feel free to reassign

The new analyzer caught a bug here, where shop-cart-modal is referencing `IronOverlayBehaviorImpl` instead of `IronOverlayBehavior`. See https://github.com/PolymerElements/iron-overlay-behavior/blob/master/iron-overlay-behavior.html for their definitions.
@frankiefu
Copy link
Member

I believe using IronOverlayBehaviorImpl was intentional.
@blasten I think you worked with @valdrinkoshi and decided IronOverlayBehaviorImpl is enough for shop-cart-modal, correct?

@valdrinkoshi
Copy link
Member

The usage was intentional indeed, since the shop-cart-modal doesn't need the other behaviors from IronOverlayBehavior (aka it doesn't need iron-resizable-behavior, iron-fit-behavior)

@blasten
Copy link
Contributor

blasten commented Dec 20, 2016

👍

@FredKSchott
Copy link
Contributor Author

I see. This is going to cause problems during linting/analysis since IronOverlayBehaviorImpl is stamping in the documentation as /** @polymerBehavior Polymer.IronOverlayBehavior */.

It feels like a hack to use just one piece of a behavior that was meant to inherit from others. Can we make this change to use the public behavior? Otherwise, if this is something we want iron-overlays-behavior to support, we need to make the change there to tag @polymerBehavior Polymer.IronOverlayBehaviorImpl.

wdyt? I'm fine with either, but if we keep things as-is shop won't be able to build using our 2.0 tool suite.

@keanulee keanulee assigned blasten and unassigned keanulee Dec 20, 2016
@valdrinkoshi
Copy link
Member

valdrinkoshi commented Dec 20, 2016

Seems like there are two @polymerBehavior tags in https://github.com/PolymerElements/iron-overlay-behavior/blob/master/iron-overlay-behavior.html
Would it help to change the specificity of @polymerBehavior Polymer.IronOverlayBehavior to just @polymerBehavior?
There is already an effort in separating this behavior into more lightweight behaviors (see PolymerElements/iron-overlay-behavior#179)

@FredKSchott
Copy link
Contributor Author

If we change @polymerBehavior Polymer.IronOverlayBehavior to just @polymerBehavior, it will resolve to the name of the behavior being assigned:Polymer.IronOverlayBehaviorImpl. This would register that behavior publicly, and then shop would be able to reference it. @valdrinkoshi should we do that instead?

@valdrinkoshi
Copy link
Member

@FredKSchott seems like swapping the two comment blocks seems to do the trick & keeps the docs correct (at least in 1.x). I guess we can close this PR and open one there

@valdrinkoshi
Copy link
Member

Opened a PR here PolymerElements/iron-overlay-behavior#224

@FredKSchott
Copy link
Contributor Author

Yup, that did it. Thanks!

@FredKSchott FredKSchott deleted the FredKSchott-patch-1 branch December 20, 2016 22:13
@frankiefu
Copy link
Member

Thanks @valdrinkoshi and @FredKSchott !

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.

5 participants