Clarify easing function requirements and clean up references to Anima… #4266

stevage · 2017-02-13T23:16:54Z

…tionOptions.

…tionOptions. Fixes mapbox#4182.

anandthakker · 2017-02-14T11:58:30Z

js/ui/camera.js

@@ -411,7 +413,8 @@ class Camera extends Evented {
     * details not specified in `options`.
     *
     * @memberof Map#
-     * @param {CameraOptions|AnimationOptions} options Options describing the destination and animation of the transition.
+     * @param {Object} options Options describing the destination and animation of the transition.
+    *            Accepts [CameraOptions](#CameraOptions) and [AnimationOptions](#AnimationOptions).


Should we stick with {CameraOptions|AnimationOptions} as the type for this parameter?

That seemed slightly misleading, because the object doesn't have to be either a CameraOptions or an AnimationOptions, but can share attributes from both.

In Flow, our type definition standard, the | operator (the "union" operator) indicates that an object may "share attributes from both: https://flowtype.org/docs/union-intersection-types.html#_

You can think of it more like the bitwise | than the logical ||

Thanks. Should have known you'd be following a standard :)

(I actually found that Flow doc page a bit ambiguous, but the page on disjoint unions makes clear that unions are non-disjoint by default)

anandthakker · 2017-02-14T12:00:46Z

js/ui/camera.js

@@ -29,7 +29,8 @@ const Evented = require('../util/evented');
 *
 * @typedef {Object} AnimationOptions
 * @property {number} duration The animation's duration, measured in milliseconds.
- * @property {Function} easing The animation's easing function.
+ * @property {Function} easing A function taking a time in the range 0..1 and returning a number where 0 is 
+ *                             the initial state and 1 is the final state.


I'm not sure / haven't checked, but it's likely that the function is expected to be nondecreasing. 🤔 I wonder if there's a non-math-class way of saying that, heh.

What makes you think that the function is expected to be nondecreasing? (I'm ok with using math class language!)

Eh, I think it was just an overly-cautious red flag instinct -- it's an assumption I could imagine making if I were implementing animations w/ a custom easing function -- but now that I look at camera.js, I don't see any obvious cases where we assume it.

I didn't test, but does the return value have to be within 0..1? Easing functions in systems like D3 generally don't.

anandthakker · 2017-02-14T12:13:52Z

js/ui/camera.js

@@ -302,7 +303,8 @@ class Camera extends Evented {
     * @param {Object} [options]
     * @param {boolean} [options.linear=false] If `true`, the map transitions using
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
-     *     {@link Map#flyTo} for information about the options specific to that animated transition.
+     *     those functions and @{link AnimationOptions} for information about options available.
+     * @param {Function} [options.easing] An easing function for the animated transition. See [AnimationOptions](#AnimationOptions).
     * @param {Function} [options.easing] An easing function for the animated transition.


options.easing is duplicated here. In fact, for consistency, would it make sense to remove options.easing (and options.offset) in favor of setting the type of [options] to {AnimationOptions}? I realize that, at present, this means more clicks for readers of the docs, but my preference would be to solve that problem somehow in the documentation generation step.

cc @lucaswoj @mollymerp - I remember a brief discussion about this, but I can't find the comment/thread.

in favor of setting the type of [options] to {AnimationOptions}?

Good idea. If we go this route, we may also want to:

rename/alias options.linear to options.animate

add options.duration (this will just be passed to Map#flyTo)

Whoops, messed up my git. I'll wait till you decide. (If you can add commits, feel free.)

I suggest we keep this PR simple and make the switch to AnimationOptions in a follow up. Would you mind ticketing this?

✅ Ticketed at #4286

anandthakker · 2017-02-14T12:15:00Z

Thanks for this, @stevage!

lucaswoj · 2017-02-14T19:39:29Z

js/ui/camera.js

@@ -29,7 +29,8 @@ const Evented = require('../util/evented');
 *
 * @typedef {Object} AnimationOptions
 * @property {number} duration The animation's duration, measured in milliseconds.
- * @property {Function} easing The animation's easing function.
+ * @property {Function} easing A function taking a time in the range 0..1 and returning a number where 0 is 
+ *                             the initial state and 1 is the final state.


What makes you think that the function is expected to be nondecreasing? (I'm ok with using math class language!)

lucaswoj · 2017-02-14T19:41:00Z

js/ui/camera.js

@@ -302,7 +303,8 @@ class Camera extends Evented {
     * @param {Object} [options]
     * @param {boolean} [options.linear=false] If `true`, the map transitions using
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
-     *     {@link Map#flyTo} for information about the options specific to that animated transition.
+     *     those functions and @{link AnimationOptions} for information about options available.


I'm 👎 on this edit. The @see tag already suggests the user may see those methods for more info. We don't need to state this explicitly.

Ok. To me it wasn't obvious that you can pass additional options and they will be respected by easeTo or flyTo respectively.

lucaswoj · 2017-02-14T19:48:09Z

js/ui/camera.js

@@ -302,7 +303,8 @@ class Camera extends Evented {
     * @param {Object} [options]
     * @param {boolean} [options.linear=false] If `true`, the map transitions using
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
-     *     {@link Map#flyTo} for information about the options specific to that animated transition.
+     *     those functions and @{link AnimationOptions} for information about options available.
+     * @param {Function} [options.easing] An easing function for the animated transition. See [AnimationOptions](#AnimationOptions).
     * @param {Function} [options.easing] An easing function for the animated transition.


in favor of setting the type of [options] to {AnimationOptions}?

Good idea. If we go this route, we may also want to:

rename/alias options.linear to options.animate

add options.duration (this will just be passed to Map#flyTo)

Clarify easing function requirements and clean up references to Anima…

a3be1a5

…tionOptions. Fixes mapbox#4182.

anandthakker reviewed Feb 14, 2017

View reviewed changes

lucaswoj reviewed Feb 14, 2017

View reviewed changes

anandthakker mentioned this pull request Feb 16, 2017

Remove redundancies in Camera method docs #4286

Closed

lucaswoj approved these changes Feb 20, 2017

View reviewed changes

Remove trailing whitespace

d9b2957

lucaswoj merged commit 1e513a4 into mapbox:mb-pages Feb 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify easing function requirements and clean up references to Anima… #4266

Clarify easing function requirements and clean up references to Anima… #4266

stevage commented Feb 13, 2017

anandthakker Feb 14, 2017

stevage Feb 14, 2017

lucaswoj Feb 16, 2017

lucaswoj Feb 16, 2017

stevage Feb 16, 2017

anandthakker Feb 14, 2017

lucaswoj Feb 14, 2017

anandthakker Feb 14, 2017

stevage Feb 14, 2017

anandthakker Feb 14, 2017

anandthakker Feb 14, 2017

lucaswoj Feb 14, 2017

stevage Feb 14, 2017

lucaswoj Feb 16, 2017

anandthakker Feb 16, 2017

anandthakker commented Feb 14, 2017

lucaswoj Feb 14, 2017

lucaswoj Feb 14, 2017

stevage Feb 14, 2017

lucaswoj Feb 14, 2017

Clarify easing function requirements and clean up references to Anima… #4266

Clarify easing function requirements and clean up references to Anima… #4266

Conversation

stevage commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment