d3 / d3-transition

Animated transitions for D3 selections.
https://d3js.org/d3-transition
ISC License
224 stars 64 forks source link

Coerce value to string for comparison #67

Closed sghall closed 7 years ago

sghall commented 7 years ago

getAttribute will always return a string. It looks like this was in there a few commits back. Not sure on the syntax, but just wanted to point it out. Could also use a template string in ES6 ${value1}.

mbostock commented 7 years ago

1. It’d be cleaner (in only one place) if you move the coercion to the call location rather than the function. That is:

(fullname.local ? attrConstantNS : attrConstant)(fullname, i, value + "")

2. You should make the equivalent fix in transition/style.js.

sghall commented 7 years ago

Right on. That should do it! I just did the commits on the interface here. Squash 'em on the merge 👍 Really liking these separate d3 modules. Nice work.

mbostock commented 7 years ago

Nice work. Thank you.

altocumulus commented 6 years ago

Note, that this breaks the algorithm used to determine the correct interpolator as described in the docs:

  1. If value is a number, use interpolateNumber.
  2. If value is a color or a string coercible to a color, use interpolateRgb.
  3. Use interpolateString.

Since the function will always be called with the new value as a string there is no way for interpolateNumber to be chosen. For a discussion of this issue have a look at "What attribute change does d3 transition include?".

mbostock commented 6 years ago

interpolateString (or interpolateRgb) is the correct behavior here. interpolateNumber should never be used for attributes.

altocumulus commented 6 years ago

@mbostock I was about to protest your initial comment until you added the second sentence to it, which seems reasonable to me ;-) If this is the desired behavior, however, the documentation needs to be adjusted as it still allows for interpolateNumber, right?