search

Polymer / shop

The Shop app
https://shop.polymer-project.org/
992 stars 494 forks source link

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

Closed FredKSchott closed 7 years ago

FredKSchott commented 7 years ago

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

frankiefu commented 7 years ago

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

valdrinkoshi commented 7 years ago

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 commented 7 years ago

👍

FredKSchott commented 7 years ago

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.

valdrinkoshi commented 7 years ago

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 https://github.com/PolymerElements/iron-overlay-behavior/issues/179)

FredKSchott commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Opened a PR here https://github.com/PolymerElements/iron-overlay-behavior/pull/224

FredKSchott commented 7 years ago

Yup, that did it. Thanks!

frankiefu commented 7 years ago

Thanks @valdrinkoshi and @FredKSchott !