clean up and add documentation #9
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # web/client/map/hooks/use-map-tool.js
mbarto
requested changes
Oct 13, 2020
| @@ -14,15 +22,19 @@ const defaultOpt = { | |||
| maxZoom: 18 | |||
| } | |||
| }; | |||
|
|
|||
| /** | |||
| * Common interface shared across multiple map types | |||
There was a problem hiding this comment.
This is not an interface, this is a Component
There was a problem hiding this comment.
yeah i know, i thought to add special category and not just call it component
going to fix it with "Locate Tool Component shared across multiple map types"
| /** | ||
| * Common interface shared across multiple map types | ||
| * @prop {object} map the map object | ||
| * @prop {string} mapType can be openlayers, leaflet or cesium |
There was a problem hiding this comment.
Why should we limit this to the actual implementations? There is nothing that prevents having a MapBoxGL api here.
There was a problem hiding this comment.
no intention to to limit maptypes here, just they are the one currently implemented
going to remove the list
| * @prop {object} map the map object | ||
| * @prop {string} mapType can be openlayers, leaflet or cesium | ||
| * @prop {string} status locate status: DISABLED, FOLLOWING, ENABLED, LOCATING, PERMISSION_DENIED | ||
| * @prop {string} message a message to show |
There was a problem hiding this comment.
Suggested change
| * @prop {string} message a message to show | |
| * @prop {string} messages message to be shown when location is found |
| * @prop {string} mapType can be openlayers, leaflet or cesium | ||
| * @prop {string} status locate status: DISABLED, FOLLOWING, ENABLED, LOCATING, PERMISSION_DENIED | ||
| * @prop {string} message a message to show | ||
| * @prop {function} changeLocateState callback to run when state changes |
There was a problem hiding this comment.
Suggested change
| * @prop {function} changeLocateState callback to run when state changes | |
| * @prop {function} changeLocateState callback to run when status changes |
| @@ -34,8 +46,13 @@ const LocateTool = ({map, mapType, status, messages, changeLocateState, onLocate | |||
| changeLocateState("DISABLED"); | |||
| }; | |||
|
|
|||
| /** | |||
| * when loaded do something, use the start method | |||
There was a problem hiding this comment.
i was thinking it could be useful for new comers but i get your point of no-comment at all 😝
| @@ -45,10 +62,23 @@ const LocateTool = ({map, mapType, status, messages, changeLocateState, onLocate | |||
| locateInstance.current?.clear(); | |||
| }; | |||
| }, [loaded]); | |||
|
|
|||
| /** | |||
| * when some of the props changes run an update | |||
| useEffect(() => { | ||
| locateInstance.current?.update({status, messages}); | ||
| }, [status, messages, loaded]); | ||
|
|
||
| /** | ||
| * when there is an error run the callbacks that handles it |
web/client/map/hooks/use-map-tool.js
Outdated
| @@ -8,7 +8,13 @@ | |||
|
|
|||
| import {useEffect, useRef, useState} from "react"; | |||
|
|
|||
| const useMapTool = (mapType, tool) => { | |||
| /** | |||
| * hook used to load in an asynchronous way a tool | |||
There was a problem hiding this comment.
Suggested change
| * hook used to load in an asynchronous way a tool | |
| * Hook that can be used to load a tool in an asynchronous way |
web/client/map/hooks/use-map-tool.js
Outdated
| const useMapTool = (mapType, tool) => { | ||
| /** | ||
| * hook used to load in an asynchronous way a tool | ||
| * @param {string} mapType can be openlayers, leaflet or cesium |
There was a problem hiding this comment.
I won't limit the possible values here
web/client/map/hooks/use-map-tool.js
Outdated
| * hook used to load in an asynchronous way a tool | ||
| * @param {string} mapType can be openlayers, leaflet or cesium | ||
| * @param {string} tool the name of the tool, should match a file in web/client/map/<mapType> folder | ||
| * @return {[boolean, object, object]} loaded if the tool has been correctly loaded, impl.current is the reference to the tool, error is an object containing the possible error that can occur while loading |
There was a problem hiding this comment.
Suggested change
| * @return {[boolean, object, object]} loaded if the tool has been correctly loaded, impl.current is the reference to the tool, error is an object containing the possible error that can occur while loading | |
| * @return {[boolean, object, object]} [loaded, tool_implementation, error]: loaded is false until the tool has been correctly loaded, tool_implementation is a ReactJS reference to the tool, error is an object containing an error if the tool implementaion cannot be loaded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean up and documentation