-
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
adding tree display to adding layer functionality #2422
adding tree display to adding layer functionality #2422
Conversation
a11b840
to
d85d3d6
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 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cphelefu, @DamonU2, and @kaminderpal)
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx
line 184 at r1 (raw file):
const populateLayerList = async (curlayerType: TypeGeoviewLayerType) => { try { const layersTree = await api.config.createMetadataLayerTree(layerURL, curlayerType, [], 'en');
I think the language should be selectable because GeoCore layers are bilingual. Just add a drop down to allow the user to select the language.
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 4 of 7 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 9 unresolved discussions (waiting on @cphelefu, @DamonU2, @kaminderpal, and @ychoquet)
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-layer-tree.tsx
line 29 at r2 (raw file):
// this use effect acts like onComponentDidMount useEffect(() => {
Add logger inside useEffect
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-layer-tree.tsx
line 45 at r2 (raw file):
useEffect(() => { onSelectedItemsChange(selectedItems);
Add logger inside useEffect
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-layer-tree.tsx
line 48 at r2 (raw file):
}, [selectedItems]); const renderTreeItem = function (layer: GroupLayerEntryConfig, parentId: string | null): JSX.Element {
CAn you add JSDoc for function
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-layer-tree.tsx
line 60 at r2 (raw file):
}; const getLayerChildren = function (treeLayerId: string): string[] {
CAn you add JSDoc for function
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx
line 184 at r1 (raw file):
Previously, ychoquet (Yves Choquette) wrote…
I think the language should be selectable because GeoCore layers are bilingual. Just add a drop down to allow the user to select the language.
Or just use the application language .... for now remove hardcoded 'en' and replace by interface language value
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx
line 218 at r2 (raw file):
// wfsValidation(); } // else if (layerType === WFS) promise = wfsValidation();
Why this dead code?
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layers-utils.ts
line 11 at r2 (raw file):
listOfLayerEntryConfig?: ListOfLayerEntry[]; }; type GeoViewLayerToAdd = {
Blank line between type
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layers-utils.ts
line 31 at r2 (raw file):
if (branchGroup.layerId === layerId) return branchGroup; const layer = branchGroup.listOfLayerEntryConfig?.find((childLayer) => childLayer.layerId === layerId);
add some comments to explain what you do and some blank line for readability (ex: between if, before for ...)
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layers-utils.ts
line 75 at r2 (raw file):
function appendChildLayerNode(treeRoot: ListOfLayerEntry, layerId: string, parentLayerId: string) { if (treeRoot.layerId === parentLayerId) {
add some comments to explain what you do and some blank line for readability (ex: between if, before for ...)
Will be merge in #2517 |
Description
This PR does the following;
Fixes #1100
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.
https://cphelefu.github.io/geoview
URL you can use for testing
https://maps-cartes.ec.gc.ca/arcgis/rest/services/CESI/MapServer/
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