-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(event): Event added to API for map ready #2510
feat(event): Event added to API for map ready #2510
Conversation
15940e5
to
0a3ed74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @DamonU2)
packages/geoview-core/src/api/api.ts
line 61 at r2 (raw file):
/** * Sets a map viewer in maps.
Missing JSDOC params. Put a bit more info to say were this is called to add and delete map. Plus a TODO to create get later (with huge impact on current users)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @DamonU2)
packages/geoview-core/src/core/app-start.tsx
line 75 at r2 (raw file):
// create a new map viewer instance and add it to the api // TODO: use store, remove the use of feature by viewer class and use state to gather values if (!(mapId in api.maps)) {
Even this check can be in api, what do you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @DamonU2)
packages/geoview-core/src/core/app-start.tsx
line 75 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Even this check can be in api, what do you think
or will it create problem.... it can stay as is and we can deal when we have the getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/app-start.tsx
line 75 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
or will it create problem.... it can stay as is and we can deal when we have the getter
We can move all of this to a function in the API or leave it as is. We need the check here so that we don't create a MapViewer if we don't need it, but moving all of this to the API makes sense too.
0a3ed74
to
6805424
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/api/api.ts
line 61 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Missing JSDOC params. Put a bit more info to say were this is called to add and delete map. Plus a TODO to create get later (with huge impact on current users)
Done.
6805424
to
ac59566
Compare
ac59566
to
2a26865
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @DamonU2)
2a03afa
into
Canadian-Geospatial-Platform:develop
Description
Added an API event to inform user when the map viewer is ready, for use outside of cgpv.init
Type of change
How Has This Been Tested?
Tested using event to log map ID in the console
https://damonu2.github.io/geoview/
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is