-
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(feature info): repairs feature info for geocore layers #2631
fix(feature info): repairs feature info for geocore layers #2631
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 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamonU2)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
// If queryable if (layerConfig.source?.featureInfo?.queryable === false) {
Why the previous was not working?
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 6 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Why the previous was not working?
It essentially turns the condition to false
when source?.queryable is undefined instead of true
by default when it's undefined.
Btw, should probably edit the comment just above the if.
Only WMS layer I know of with HTML return on feature info: 6433173f-bca8-44e6-be8e-3e8a19d3c299 |
604ee4f
to
edcb3a7
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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @DamonU2)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
It essentially turns the condition to
false
when source?.queryable is undefined instead oftrue
by default when it's undefined.
Btw, should probably edit the comment just above the if.
We shuold have the warning even when undefined.... should we revert
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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @DamonU2)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
We shuold have the warning even when undefined.... should we revert
If we need to continue when undefined, we need a GV comment to explain why we should do this
2da198b
to
6dff860
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: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @jolevesq)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
It essentially turns the condition to
false
when source?.queryable is undefined instead oftrue
by default when it's undefined.
Btw, should probably edit the comment just above the if.
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 1 of 1 files at r3, all commit messages.
Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
Done.
GV need to be capitalize letter.... Do we want logger.error? Just looger.warning should be enough?
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 3 files at r4.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)
6dff860
to
c3abc28
Compare
c3abc28
to
88085a1
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: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @jolevesq)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 255 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
GV need to be capitalize letter.... Do we want logger.error? Just looger.warning should be enough?
Done. Small gv actually works the same.
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 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamonU2)
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 @DamonU2)
7fb9bb5
into
Canadian-Geospatial-Platform:develop
Closes #2593
Closes #2210
Closes #563
Description
Fixes some issues with getting the feature info for some geocore layers. Adds support for html response from WMS getFeatureInfo.
Type of change
How Has This Been Tested?
https://damonu2.github.io/geoview/add-layers.html
HTML WMS: 6433173f-bca8-44e6-be8e-3e8a19d3c299
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