Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document or remove any event that can be fired by "Map" #2232

Closed
lucaswoj opened this issue Mar 9, 2016 · 17 comments
Closed

Document or remove any event that can be fired by "Map" #2232

lucaswoj opened this issue Mar 9, 2016 · 17 comments
Assignees

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Mar 9, 2016

There are many events that we fire but don't consider part of our public API. Many users are using these events despite them being undocumented. We should consider prefixing these events with an _, like a private method, to discourage their use.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Mar 9, 2016

The biggest offender is style.load. I see a lot of uses of the undocumented style.load where load should have been used. I suspect many people started using style.load as a workaround for #2042.

@jfirebaugh
Copy link
Contributor

I'd be fine with publicly documenting style.load and pretty much all the *.* events.

@tmcw
Copy link
Contributor

tmcw commented Mar 9, 2016

Agree with @jfirebaugh, if an event isn't public, we shouldn't emit it.

@lucaswoj lucaswoj changed the title Prefix "private" events with an underscore Document or remove any event that can be fired by "Map" Mar 9, 2016
@lucaswoj
Copy link
Contributor Author

lucaswoj commented Mar 9, 2016

👍 Makes sense. I adjusted the issue title to reflect this approach.

@lyzidiamond
Copy link
Contributor

Here is a list of all of the events that are undocumented and where they live in the code. Some of them may be undocumented intentionally and the link I put here may not be the canonical link. Tell me which should be documented and which should be removed!

@bhousel
Copy link
Contributor

bhousel commented Mar 19, 2016

Wow thanks @lyzidiamond! I can help with this..

I just added the touch* events within the past few days, and they are documented, that code just hasn't made it into a release yet.

@lyzidiamond
Copy link
Contributor

@bhousel ah yes, i saw the doc comments in the code and was confused as to why they weren't showing in the docs! Comment above updated.

@lyzidiamond
Copy link
Contributor

@bhousel also I'm happy to add the doc, just need to know which ones to document!

@lyzidiamond
Copy link
Contributor

A bunch more here, including the source.*, layer.* and tile.* events. I'm not sure where to find the style.* events.

mapbox-gl-js/js/ui/map.js

Lines 508 to 527 in 2028b48

this.style
.on('load', this._onStyleLoad)
.on('error', this._forwardStyleEvent)
.on('change', this._onStyleChange)
.on('source.add', this._onSourceAdd)
.on('source.remove', this._onSourceRemove)
.on('source.load', this._onSourceUpdate)
.on('source.error', this._forwardSourceEvent)
.on('source.change', this._onSourceUpdate)
.on('layer.add', this._forwardLayerEvent)
.on('layer.remove', this._forwardLayerEvent)
.on('layer.error', this._forwardLayerEvent)
.on('tile.add', this._forwardTileEvent)
.on('tile.remove', this._forwardTileEvent)
.on('tile.load', this._update)
.on('tile.error', this._forwardTileEvent)
.on('tile.stats', this._forwardTileEvent);
this.on('rotate', this.style._redoPlacement);
this.on('pitch', this.style._redoPlacement);

@lucaswoj
Copy link
Contributor Author

I'm not sure where to find the style.* events.

The style. prefix is added here.

Style events are fired throughout the js/style/style.js file. For example, style.load is fired as load here.

@andreasplesch
Copy link

As somebody who did some ol3, I was actually looking for map generated events and was wondering if there were any because they are not in listed in the API documentation. In particular, I was looking for an event which is fired whenever a render is completed. map#render seems to fit. It would be great if these events could be documented, even if very briefly. Otherwise adopters will just try to find them in the code. Even a list of available events without documentation (such as above) is very helpful.
(Is there a canonical way to sync across two map canvases ?)

@lyzidiamond
Copy link
Contributor

@lucaswoj @bhousel @jfirebaugh any idea which of the events i listed should have doc and which should be removed? Happy to hit this once I know which is which.

@lucaswoj
Copy link
Contributor Author

Of the events listed, I think we should document render and error. I don't have any strong opinions about the rest.

@jfirebaugh
Copy link
Contributor

I think these should all be documented except for tile.stats, which is an internal performance diagnostic thing.

@lucaswoj
Copy link
Contributor Author

I think we should clean up our events architecture by doing the following things:

Already Done

  • document touchstart
  • document touchmove
  • document touchend
  • document touchcancel

Step 1

  • document pitch
  • document render

Step 2

  • document error
  • merge tile.error with error
  • merge style.error with error
  • merge source.error with error
  • merge tile.error with error
  • merge layer.error with error

Step 3

  • add pitchstart and pitchend events
  • ensure rotatestart and rotateend events are fired in all cases

Step 4

  • implement and document data and dataend (ref Add data, datastart and dataend events #1715 (comment))
  • remove tile.load in favor of data
  • remove tile.stats in favor of data
  • remove tile.add in favor of data
  • remove tile.remove in favor of data
  • remove source.change in favor of data

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Sep 23, 2016

Revisiting this at a high level. Now that #3244 has landed we have better visibility into our events:

Events fired in GL JS

  • boxzoomcancel
  • boxzoomend
  • boxzoomstart
  • click
  • close
  • contextmenu
  • dblclick
  • drag
  • dragend
  • dragstart
  • error
  • geolocate
  • layer.add
  • layer.remove
  • load
  • mousedown
  • mousemove
  • mouseout
  • mouseup
  • move
  • moveend
  • movestart
  • pitch
  • remove
  • render
  • resize
  • rotate
  • rotateend
  • rotatestart
  • source.add
  • source.change
  • source.load
  • source.remove
  • style.change
  • style.load
  • tile.add
  • tile.load
  • tile.remove
  • touchcancel
  • touchend
  • touchmove
  • touchstart
  • webglcontextlost
  • webglcontextrestored
  • zoom
  • zoomend
  • zoomstart

Events Documented in GL JS

  • boxzoomcancel
  • boxzoomend
  • boxzoomstart
  • click
  • close
  • contextmenu
  • dblclick
  • drag
  • dragend
  • dragstart
  • error
  • geolocate
  • load
  • mousedown
  • mousemove
  • mouseout
  • mouseup
  • move
  • moveend
  • movestart
  • pitch
  • render
  • rotate
  • rotateend
  • rotatestart
  • touchcancel
  • touchend
  • touchmove
  • touchstart
  • webglcontextlost
  • webglcontextrestored
  • zoom
  • zoomend
  • zoomstart

Events Fired but not Documented

  • remove
  • resize

Events to be Merged into the data Event

  • layer.add
  • layer.remove
  • source.add
  • source.change
  • source.load
  • source.remove
  • style.change
  • style.load
  • tile.add
  • tile.load
  • tile.remove

This was referenced Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants