search

PolymerElements / paper-behaviors

Common behaviors used across paper-* elements.
19 stars 27 forks source link

Correct `noink` property to `no-ink` to actually conform to established property name conventions #69

Open lozandier opened 8 years ago

lozandier commented 8 years ago

The problem

While sorta funny & daresay "cute", noink seemingly seriously violates the conventions expected for property names that other elements conform to. Seeing no reason that it be a special exception, this PR changes the noink boolean property to no-ink

If noink is still used—or existing instances using this behavior are still using it— its observer will instead delegate the changes to the correct observers associated with no-ink while providing a console warning that the use of noink is deprecated.

Edit: To clarify, these changes enable noink to be used now by existing consumers of the component while communicating that it is on its way out of the API—effectively starting a phase-out period.

This enables the current behavior while informing consumers of this component that noink should no longer be relied on once 1.0 is bumped to 1.1.

Side-effects

Motivated in a way by @notwaldorf about APIs (always wanted to go ahead and do this change but haven't had moments often that I had to rely on this behavior until now), it'd be awesome she reviewed it along with the other 3 recent reviewers of this repo:

Leveraged existing tests given the changes made; I can see an argument that noInk method be mocked & ensured to be called when noink is still used & so on…

Estimated Review Time

keanulee commented 8 years ago

Removing noink would be a breaking change, so in keeping with semver, it wouldn't be appropriate for a feature (1.1) release and would have to be a major (e.g. 2.0) release. While I agree that no-ink is more consistent with other property/attribute names, I don't think this is worth breaking/deprecating existing users on at least on this major version (1.x).

lozandier commented 8 years ago

@keanulee This change doesn't break the use of noink right now, rather it warns that's it's on its way out that I'm of the opinion make sense given the existing property name conventions established (including w/ SVGs as well).

Being a minor change & keeping the existing behavior while leaving room for its succession, I thought 1.0.12 would be appropriate.

Having a hunch that it may very well be something that is removed altogether with 2.x rather than 1.11 despite being a typo of an existing method, I edited my error message to not yet get into detail what version will remove noink

abdonrd commented 8 years ago

LGTM! :)

lozandier commented 8 years ago

@notwaldorf @abdonrd @keanulee Stylistic & ultimately out-of-scope changes of this PR removed; feel free to give me a LGTM (in the case of @abdonrd, perhaps again) when you're next free for a LGTM combo breaker.

lozandier commented 8 years ago

@keanulee To confirm, your concerns about 1.1 vs. `2.0 has been addressed.

notwaldorf commented 8 years ago

/cc @cdata since this is a bigger discussion that this PR, imo (other elements have a noink property). Also: noink is currently being used for styling in a bunch of places (paper-ripple)

cdata commented 8 years ago

Thanks for raising this point, @lozandier.

noink is a (unfortunate) carry-over from Polymer 0.5 element conventions. Frankly, it bothers me to no end every time I have to think about this attribute.

A rename amounts to a breaking API change (in turn asking for a major version bump), and regardless should be done in a coordinated fashion across all elements with the noink attribute. We should address this by changing noink to noInk per our convention as we upgrade elements to 2.0, but in the mean time we cannot break the de facto convention that asks that noink be spelled the way it is.

lozandier commented 8 years ago

@cdata Right, I totally get that. Accordingly, I made sure to account for that yet have the element edited to encourage uses of the element moving forward to correctly use no-ink instead of noink while not immediately breaking existing instances that use noink instead.

The end consumer will see the console warning with their use or end users of the document that uses the behavior via noink that implicitly encourage them to change to no-ink superficially in the meantime.

abdonrd commented 8 years ago

I'm totally agree display a warning for now, and make the break change in the v2.0.

lozandier commented 8 years ago

@cdata @notwaldorf @abdonrd @keanulee Any update on this? It seems we're all on the same page to make sure this isn't a breaking change (AFAIK, it isn't; it merely warns them that the component needs to own up from its unfortunate, humbling beginning to use noink instead of no-ink).

lozandier commented 8 years ago

/cc @garlicnation @tjsavage

tjsavage commented 8 years ago

This change totally makes sense to me, to build a bridge for breaking changes we know we're going to introduce in a potential 2.0 of the elements. I agree with @cdata though that we should probably make this change across all elements that use noink at the same time. The way I think about it is that if all these elements were in a single repo, we would definitely do all of them at once.

Just taking a glance over some of the other repos with noink, it looks like some of them have bugs with how noink was applied as well.