-
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
refactor(store): Refactor north arrow to use the store and remove api… #1358
refactor(store): Refactor north arrow to use the store and remove api… #1358
Conversation
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 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @cphelefu, @jolevesq, and @ychoquet)
packages/geoview-core/src/api/eventProcessors/map-event-process.ts
line 51 at r1 (raw file):
// because emit and on from api events can be trigger in loop, compare also the api value if (cur !== prev && api.maps[mapId].currentProjection !== cur!) { console.log(`proj emit ${mapId}`);
"Disable no console for this line" to prevent flags?
packages/geoview-core/src/api/eventProcessors/map-event-process.ts
line 84 at r1 (raw file):
// because emit and on from api events can be trigger in loop, compare also the api value if (payloadIsAMapViewProjection(payload) && api.maps[mapId].currentProjection !== payload.projection!) { console.log(`proj on ${mapId}`);
As above
packages/geoview-core/src/core/components/overview-map/overview-map.tsx
line 113 at r1 (raw file):
/** * Return the map ovberview map
ovberview > overview
packages/geoview-core/src/core/stores/stores-managers.ts
line 35 at r1 (raw file):
initializeEventProcessors(geoViewStore); console.log('store creation');
Disable no console?
packages/geoview-core/src/geo/map/map.ts
line 413 at r1 (raw file):
this.map.getView().on('change:rotation', store.getState().mapState.onMapRotation); console.log('store add map event');
As above
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 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jolevesq and @ychoquet)
eaa360e
to
8c7cab4
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 7 of 13 files at r2.
Reviewable status: 19 of 25 files reviewed, 5 unresolved discussions (waiting on @cphelefu, @DamonU2, and @ychoquet)
packages/geoview-core/src/api/eventProcessors/map-event-process.ts
line 51 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
"Disable no console for this line" to prevent flags?
Done.
packages/geoview-core/src/api/eventProcessors/map-event-process.ts
line 84 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
As above
Done.
packages/geoview-core/src/core/components/overview-map/overview-map.tsx
line 113 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
ovberview > overview
Done.
packages/geoview-core/src/core/stores/stores-managers.ts
line 35 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
Disable no console?
Done.
packages/geoview-core/src/geo/map/map.ts
line 413 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
As above
Done.
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 14 of 21 files at r1, 4 of 13 files at r2, all commit messages.
Reviewable status: 23 of 25 files reviewed, 5 unresolved discussions (waiting on @cphelefu, @DamonU2, and @ychoquet)
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 13 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ychoquet)
fe12955
to
f79b1f0
Compare
Why do we have this here? Where do we call this NOTIFICATION_ADD; why didnt we just add it to the store directly from there? |
This is going to refresh page whenever any property in mapState is changed. Something along these lines, confirm in zustand documentation |
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 14 of 14 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ychoquet)
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: all files reviewed, 2 unresolved discussions (waiting on @cphelefu and @ychoquet)
packages/geoview-core/src/api/eventProcessors/notification-event-process.ts
line 15 at r3 (raw file):
Previously, cphelefu (Christopher Phelefu) wrote…
Why do we have this here? Where do we call this NOTIFICATION_ADD; why didnt we just add it to the store directly from there?
Because this event may be emit by the api from outside the viewer
… from footer element Closes Canadian-Geospatial-Platform#1329
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 13 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cphelefu and @ychoquet)
f79b1f0
to
ff002dd
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 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @cphelefu and @ychoquet)
packages/geoview-core/src/core/components/map/map.tsx
line 59 at r3 (raw file):
Previously, cphelefu (Christopher Phelefu) wrote…
This is going to refresh page whenever any property in mapState is changed.
I think it should be (state) => [state.mapState.mapLoaded, state.mapState,northArrow ])..Something along these lines, confirm in zustand documentation
Done.
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 all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cphelefu and @ychoquet)
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 8 of 13 files at r2, 13 of 14 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ychoquet)
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: complete! all files reviewed, all discussions resolved (waiting on @ychoquet)
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 21 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ychoquet)
79f6686
into
Canadian-Geospatial-Platform:develop
… from footer element
Closes #1329
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Add the URL for your deploy!
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