-
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
fix(data-table): Show click marker on zoom to feature like details #2105
fix(data-table): Show click marker on zoom to feature like details #2105
Conversation
d23252a
to
2cc22ac
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 9 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jolevesq and @kaminderpal)
packages/geoview-core/src/app.tsx
line 90 at r2 (raw file):
// configurations file url is provided, fetch then process const configUrl = mapElement.getAttribute('data-config-url'); const configObject = await fetchConfigFile(configUrl!);
Is it possible to change the return type of the fetchConfigFile
to Promise and do the as unknown as MapFeatureConfig
intrinsically in order to minimise the spreading of those?
packages/geoview-core/src/core/components/data-table/data-table.tsx
line 357 at r2 (raw file):
// Zoom to extent and wait for it to finish zoomToExtent(extent)
I remember having to do this sort of weird patch. I see it copied here. Is it because it's been moved here or do we have duplication of the patch and it's starting to spread?
If it's spreading, I'd suggest reviewing the design and usage.
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 9 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan and @kaminderpal)
packages/geoview-core/src/app.tsx
line 90 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Is it possible to change the return type of the
fetchConfigFile
to Promise and do theas unknown as MapFeatureConfig
intrinsically in order to minimise the spreading of those?
The as uknow is a patch that will be remove once we have the new config for layer. I have a todo to remove this code
packages/geoview-core/src/core/components/data-table/data-table.tsx
line 357 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
I remember having to do this sort of weird patch. I see it copied here. Is it because it's been moved here or do we have duplication of the patch and it's starting to spread?
If it's spreading, I'd suggest reviewing the design and usage.
You are right, I copied the path... will add a TODO
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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kaminderpal)
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 @kaminderpal)
1282288
into
Canadian-Geospatial-Platform:develop
Closes #1933
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.
Add click marker on zoom like in detail
Fixes #1933
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.
Hosted, May 9th - 7AM: https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/06-basic-footer.json
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