-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(geocore): better language switching for geocore #2717
base: develop
Are you sure you want to change the base?
feat(geocore): better language switching for geocore #2717
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 11 of 14 files at r1, all commit messages.
Reviewable status: 11 of 14 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @MatthewMuehlhauserNRCan)
packages/geoview-core/public/locales/en/translation.json
line 229 at r1 (raw file):
"noTab": "No tab" } }
Why does your prettier did this. There a config you have and who is not in the repo. Same for next translation file
packages/geoview-core/src/core/utils/config/config.ts
line 65 at r1 (raw file):
this.#setLayerEntryType( geoViewLayerEntryCasted.listOfLayerEntryConfig!, geoViewLayerEntryCasted.geoviewLayerType as TypeGeoviewLayerType
Why do we need to cast the type now
packages/geoview-core/src/geo/map/map-viewer.ts
line 1252 at r1 (raw file):
* @param {boolean} maintainGeocoreLayerNames - Indicates if geocore layer names should be kept as is or returned to defaults. */ reloadWithCurrentState(maintainGeocoreLayerNames: boolean = true): void {
Is this parameter mostly valid for language change? If the reload state stays in the same language it would always be true? Should we rename the param to reflect or add comment in the JSDOC
packages/geoview-core/src/geo/map/map-schema-types.ts
line 374 at r1 (raw file):
metadataAccessPath?: string; /** Type of GeoView layer. */ geoviewLayerType: 'geoCore' | TypeGeoviewLayerType;
We should not modify the map-schema type in geo. We should use as much as we could the one in config. This one is deprecated. In new config there is flag to know if it is geocore originally ( "isGeocore": true,). Should we use same approach?
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 33 at r1 (raw file):
import { TypeGeoviewLayerType, TypeOutfieldsType } from '@/api/config/types/map-schema-types'; const translations = {
Translation should be in translation file and we use the getLocalizedMessage function in utilities warning AND like you did with showError but look at how it is done with other error were parameter are pass to the function (i.e. wrongPath)
packages/geoview-core/src/geo/layer/other/geocore.ts
line 59 at r1 (raw file):
tempLayerConfig.metadataAccessPath = response.layers[0].metadataAccessPath; tempLayerConfig.geoviewLayerType = response.layers[0].geoviewLayerType; if (!tempLayerConfig.geoviewLayerName) tempLayerConfig.geoviewLayerName = response.layers[0].geoviewLayerName;
Can you add a comment why you do this if
1985fed
to
9820dd3
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 3 of 14 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan and @DamonU2)
9820dd3
to
bebf801
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: 10 of 14 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, and @MatthewMuehlhauserNRCan)
packages/geoview-core/src/geo/layer/gv-layers/abstract-gv-layer.ts
line 33 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Translation should be in translation file and we use the getLocalizedMessage function in utilities warning AND like you did with showError but look at how it is done with other error were parameter are pass to the function (i.e. wrongPath)
Done.
packages/geoview-core/src/geo/layer/other/geocore.ts
line 59 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Can you add a comment why you do this if
Done.
packages/geoview-core/src/geo/map/map-schema-types.ts
line 374 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
We should not modify the map-schema type in geo. We should use as much as we could the one in config. This one is deprecated. In new config there is flag to know if it is geocore originally ( "isGeocore": true,). Should we use same approach?
It's deprecated but we still need the types until the config is handling this.
packages/geoview-core/src/core/utils/config/config.ts
line 65 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Why do we need to cast the type now
I changed how I was handling the config so this is no longer needed. Removed.
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 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)
4f7c369
to
d261d2d
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: 13 of 18 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, and @MatthewMuehlhauserNRCan)
packages/geoview-core/src/geo/map/map-viewer.ts
line 1252 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this parameter mostly valid for language change? If the reload state stays in the same language it would always be true? Should we rename the param to reflect or add comment in the JSDOC
Done.
d932aa0
to
8acfb0f
Compare
8acfb0f
to
8d470ad
Compare
Closes #2711
Description
Adds a variable to createMapConfigFromMapState to drop geocore layer names to pick them up from the source in a different language. Also adds a translation to image loading error and fixes an issue with names of group layers.
Type of change
How Has This Been Tested?
https://damonu2.github.io/geoview/demo-osdp-integration.html
5a65ad7c-561a-466a-8375-8d876624df9d
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