Update the documentation in src/map.js. #698

manthey · 2017-05-19T19:55:24Z

Only the first 1/3 has been done, mainly to get feedback if this is what we want.

I've removed a few bits of vestigial code -- map.scale and map.maxZoom were unused. The pan event claimed it included more information than it did, and included an extra empty element.

We could get rid of the map.origin() code -- we don't use it or test it other than simple coverage.

Perhaps we should have a more central place to document datatypes used in the documentation -- many are in the mapInteractor code.

Only the first 1/3 has been done, mainly to get feedback if this is what we want. I've removed a few bits of vestigial code -- map.scale and map.maxZoom were unused. The pan event claimed it included more information than it did, and included an extra empty element. We could get rid of the map.origin() code -- we don't use it or test it other than simple coverage. Perhaps we should have a more central place to document datatypes used in the documentation -- many are in the mapInteractor code.

manthey · 2017-05-19T19:57:01Z

@jbeezley and @aashish24 Here is my first take on trying to update map.js's documentation. I've only done the first 1/3 of the file or so, since I want to make sure we are happy with how things are being done. I have questions about how jsdoc generates event documentation (see my comment on issue #448).

codecov-io · 2017-05-19T20:03:23Z

Codecov Report

Merging #698 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
- Coverage   95.28%   95.28%   -0.01%     
==========================================
  Files          83       83              
  Lines        8990     8988       -2     
==========================================
- Hits         8566     8564       -2     
  Misses        424      424

Impacted Files	Coverage Δ
src/mapInteractor.js	`96.12% <ø> (ø)`	⬆️
src/index.js	`100% <ø> (ø)`	⬆️
src/event.js	`100% <ø> (ø)`	⬆️
src/map.js	`98.78% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 276bb52...8fbac1d. Read the comment docs.

I've moved the typedefs in map and mapInteractor to their own file. In principle, the documentation in src/map.js is done (barring typos). I've fixed some documentation that was missing or wrong. I rearranged some of the private inner functions to group them together and keep them above the initialization code.

manthey · 2017-05-24T15:37:53Z

I've gone through all of src/map.js. Criticism welcome.

aashish24 · 2017-05-30T15:36:57Z

src/map.js

-   *    the new view.
-   * @returns {Object|geo.map}
+   * @param {boolean|'limited'} [ignoreClampBounds] if true ignore the
+   *    clampBounds* options when determining the new view.  When 'limited',


why star after clampBounds?

shortened form of clampBoundsX and clampBoundsY options. Obviously not clear enough, so I shouldn't use the shortened form

aashish24 · 2017-05-30T15:37:27Z

src/map.js

@@ -596,8 +597,9 @@ var map = function (arg) {
  /**
   * Remove layer from the map
   *
-   * @param {geo.layer} layer that should be removed from the map
-   * @return {geo.map}
+   * @param {geo.layer?} layer layer that should be removed from the map


was ? intended?

aashish24 · 2017-05-30T15:38:27Z

src/map.js

@@ -835,7 +819,9 @@ var map = function (arg) {

  ////////////////////////////////////////////////////////////////////////////
  /**
-   * Manually force to render map
+   * Redraw the map and all its layers.


aashish24

Mostly looking reasonable to me but I may have missed few things. I think the new doc is much better than the last version.

aashish24 · 2017-05-30T15:43:24Z

@jbeezley @manthey should we have a style doc somewhere that can summarize @jbeezley suggestions?

jbeezley · 2017-05-30T17:26:45Z

src/map.js

@@ -667,12 +669,13 @@ var map = function (arg) {

  ////////////////////////////////////////////////////////////////////////////
  /**
-   * Resize map (deprecated)
+   * Resize map in pixels.


Do we want to consider this deprecated? I don't know that calling this with x or y nonzero actually works. If so, we should add a @deprecated and remove it on the next breaking change release.

We use this function when the window is resized via the resizeSelf function. We don't do anything with x and y (just with width and height).

Since prior to this PR, it has been labelled deprecated for a while, we could remove the function and roll the needed functionality into resizeSelf. We could remove x and y from the geo.event.resize event properties, too.

jbeezley · 2017-05-30T17:28:02Z

src/map.js

+   * @param {number} y y-offset in display space
+   * @param {number} w width in display space
+   * @param {number} h height in display space
+   * @fires geo.event.resize


For some reason, the link doesn't work for this declaration. The same is true for a number of events added in this file.

jbeezley · 2017-05-30T17:43:47Z

src/map.js

+   *    clampBoundsX and clampBoundsY options when determining the new view.
+   *    When 'limited', the clampBoundsX and clampBoundsY options are
+   *    selectively enforced so that the map will not end up more out of bounds
+   *    than its current state.


In places like this where there are literals used in prose (e.g. true, 'limited', and clampBoundsX ), I think it reads better to display them in monospace. This follows the style used in the openlayers documentation as well. You can use markdown syntax to accomplish it. Should we add this convention to the checklist?

jbeezley · 2017-05-30T17:50:16Z

src/map.js

@@ -357,14 +341,18 @@ var map = function (arg) {
  /**
   * Get/Set zoom level of the map
   *
-   * @param {number} val if undefined, return the current zoom level.
+   * @param {number} [val] if undefined, return the current zoom level.


Our parameter descriptions are inconsistent with regard to capitalization and ending in a period. I don't know which we should prefer, but ideally it would the same everywhere. For comparison, openlayers uses a capital letter and period for all parameter descriptions (even those that aren't complete sentences).

Let us do that, too. I'd say the only exception is when the first word is a literal, in which case it can be in monospace.

jbeezley · 2017-05-30T18:06:50Z

src/map.js

+   * @param {string|object} [readerOrName]: undefined to get the current
+   *    reader, an instance of a file reader to set the reader, or a name to
+   *    create a file reader.
+   * @param {object} [opts]: options for creating a file reader when the reader


It doesn't seem to change the output here, but for consistency it would be good to remove the : in these declarations.

I meant to remove the :.

All parameter descriptions are capitalized and end with punctuation. All events are defined. Literals are mark-down quoted to be monospace. Fixed some wording.

manthey · 2017-05-30T19:49:48Z

@jbeezley Short of refactoring or removing resize, I've addressed your comments. I'll note that resize is also called from the size method, so it might make more sense to move its code there and have resizeSelf call size.

aashish24 · 2017-05-31T19:14:16Z

@manthey I am fine with changes, although I wish we had a doc style checker. Manually checking doc changes seems harder than it looks.

@jbeezley are we still waiting on changes?

jbeezley · 2017-06-05T11:34:39Z

src/map.js

-   *    instead of cancelAnimationFrame).  Any other truthy value moves the
-   *    callback to the end of the list.
-   * @returns {integer} An integer as returned by window.requestAnimationFrame.
+   * @param {function} Xallback function to call during the animation frame.


jbeezley · 2017-06-05T11:36:38Z

src/map.js

+   * @param {object} [arg] The zoom range.
+   * @param {number} [arg.min] The minimum zoom level.
+   * @param {number} [arg.max] The maximum zoom level.
+   * @param {boolean} [noRefresh] if `true`, don't update the map if the zoom


Capitalize and end in a period.

jbeezley · 2017-06-05T12:14:24Z

src/map.js

@@ -1750,7 +1750,7 @@ var map = function (arg) {
   * callbacks will be called in a single time slice, providing better
   * synchronization.
   *
-   * @param {function} Xallback function to call during the animation frame.
+   * @param {function} Callback function to call during the animation frame.


The parameter name is "callback", so this should be "callback Function".

aashish24 · 2017-06-05T13:38:48Z

🎉

manthey force-pushed the map-class-documentation branch from a7066c0 to 340b2d3 Compare May 24, 2017 15:37

aashish24 reviewed May 30, 2017

View reviewed changes

Make a few changes to the documentation based on suggestions.

7f2eab3

jbeezley suggested changes May 30, 2017

View reviewed changes

More small docs adjustments.

71b3199

All parameter descriptions are capitalized and end with punctuation. All events are defined. Literals are mark-down quoted to be monospace. Fixed some wording.

Merge branch 'master' into map-class-documentation

38c3015

jbeezley suggested changes Jun 5, 2017

View reviewed changes

Fix some documentation typos.

85e857e

jbeezley reviewed Jun 5, 2017

View reviewed changes

Fix the previous fix.

8fbac1d

jbeezley approved these changes Jun 5, 2017

View reviewed changes

manthey merged commit e57e648 into master Jun 5, 2017

manthey deleted the map-class-documentation branch June 5, 2017 12:30

jbeezley mentioned this pull request Jun 12, 2017

Update eslint and enable linting of docstrings #706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the documentation in src/map.js. #698

Update the documentation in src/map.js. #698

manthey commented May 19, 2017

manthey commented May 19, 2017

codecov-io commented May 19, 2017 •

edited

Loading

manthey commented May 24, 2017

aashish24 May 30, 2017

manthey May 30, 2017

aashish24 May 30, 2017

aashish24 May 30, 2017

aashish24 left a comment

aashish24 commented May 30, 2017

jbeezley May 30, 2017

manthey May 30, 2017

jbeezley May 30, 2017

jbeezley May 30, 2017

manthey May 30, 2017

jbeezley May 30, 2017

manthey May 30, 2017

jbeezley May 30, 2017

manthey May 30, 2017

manthey commented May 30, 2017 •

edited

Loading

aashish24 commented May 31, 2017

jbeezley Jun 5, 2017

jbeezley Jun 5, 2017

jbeezley Jun 5, 2017

aashish24 commented Jun 5, 2017

Update the documentation in src/map.js. #698

Update the documentation in src/map.js. #698

Conversation

manthey commented May 19, 2017

manthey commented May 19, 2017

codecov-io commented May 19, 2017 • edited Loading

Codecov Report

manthey commented May 24, 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

aashish24 left a comment

Choose a reason for hiding this comment

aashish24 commented May 30, 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

manthey commented May 30, 2017 • edited Loading

aashish24 commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aashish24 commented Jun 5, 2017

codecov-io commented May 19, 2017 •

edited

Loading

manthey commented May 30, 2017 •

edited

Loading