-
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(table): EsriDynamic does not have right icon after no fetch of ge… #2590
fix(table): EsriDynamic does not have right icon after no fetch of ge… #2590
Conversation
e3a6a47
to
68c77cf
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 8 of 12 files at r2, 2 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/geo/layer/gv-layers/raster/gv-esri-dynamic.ts
line 128 at r4 (raw file):
// Guess the geometry type by taking the first style key // TODO: Refactor - Layers migration. Johann: This will be modified with new schema, there is no more geometry on style const [geometryType] = layerConfig.getTypeGeometries();
Nice cleanup
packages/geoview-core/src/geo/utils/renderer/geoview-renderer.ts
line 1603 at r4 (raw file):
let canvas: HTMLCanvasElement | undefined; // GV: Some time, the feature will have no geometry e.g. esriDynamic has we fetch geometry only when needed
Minor typos:
Some time => Sometimes
has we => as we
packages/geoview-core/src/geo/utils/renderer/geoview-renderer.ts
line 1600 at r4 (raw file):
legendFilterIsOff?: boolean, useRecycling?: boolean, callbackForDataUrl?: () => Promise<string | null>
Si tu as le temps de m'en parler un peu, je serais curieux de voir comment tu as remplacé ce callback. Il me semble que ça servait? Ça ne ramasse plus le legendIcons![0].iconImage
dans abstract-geoview-layers.formatFeatureInfoResult()
et abstract-gv-layers
?
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 12 files at r2, 2 of 4 files at r3, 3 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/api/event-processors/event-processor-children/legend-event-processor.ts
line 696 at r5 (raw file):
// Sort class breaks by minValue for binary search // GV: Vlaues can be number, date, string, null or undefined. Should it be only Date or Number
Vlaues > Values
packages/geoview-core/src/api/event-processors/event-processor-children/legend-event-processor.ts
line 697 at r5 (raw file):
// Sort class breaks by minValue for binary search // GV: Vlaues can be number, date, string, null or undefined. Should it be only Date or Number // GV: undefine or null should not be allowed in class break style
undefine > undefined
d0c0261
to
5efc527
Compare
d7b4fc4
into
Canadian-Geospatial-Platform:develop
…ometry
Closes #2585-table-icon-dynamic
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.
Since we do not fetch geometry for esriDynamic, the geoview-renderer defaulted to first icon for all feature.
Fixes #2585, #2536
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 November 10th: 11:20
https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/16-esri-dynamic.json
https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/17-esri-feature.json
https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/19-geojson.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