Skip to content
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

2471-Comple_layer_tree_coverage #2473

Conversation

ychoquet
Copy link
Member

@ychoquet ychoquet commented Sep 11, 2024

Description

This issue covers two aspects of the layer tree implementation;

1- Improved tree generation for the WMS type on the Geomet site;
2- Temporary tree implementation for GeoView layers not yet coded.

Fixes #2471

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Using Chrome Devtools and https://ychoquet.github.io/GeoView/config-sandbox.html template

Deploy URL: https://ychoquet.github.io/GeoView/config-sandbox.html

Checklist:

  • I have build (rush build) and deploy (rush host) my PR
  • I have connected the issues(s) to this PR
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have created new issue(s) related to the outcome of this PR is needed
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

This change is Reviewable

@ychoquet ychoquet self-assigned this Sep 11, 2024
Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan and @ychoquet)


packages/geoview-core/src/api/config/config-api.ts line 527 at r1 (raw file):

    language: TypeDisplayLanguage = 'en'
  ): Promise<EntryConfigBaseClass[]> {
    // GV: TEMPORARY SECTION TO BE DELETED WHEN ALL LAYER TYPES ARE IMPLEMENTED

Can you just add a small comment to say why is it temporary


packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 69 at r1 (raw file):

          jsonMetadata = JSON.parse(metadataString);
        } catch (error) {
          logger.logError('HTTP request returned an invalid JSON string.\n', error);

Should it be HTTPS? HTTP is not suppose to be supported


packages/geoview-core/src/api/config/types/classes/geoview-config/raster-config/wms-config.ts line 54 at r1 (raw file):

      this.setErrorDetectedFlag();
      logger.logError(`Invalid metadataAccessPath.\nmetadataAccessPath="${this.metadataAccessPath}"`);
    } else if (metadataAccessPathItems.length === 2) {

What if === 1... Maybe it is reviewable but I do not see a else after this condition

@ychoquet ychoquet force-pushed the 2471-Complete_layer_tree_coverage branch from c66bf26 to 014ea2e Compare September 12, 2024 18:06
Copy link
Member Author

@ychoquet ychoquet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, and @ychoquet)


packages/geoview-core/src/api/config/config-api.ts line 527 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Can you just add a small comment to say why is it temporary

Done.


packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 69 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should it be HTTPS? HTTP is not suppose to be supported

Done. Changed it for a message that doesn't mention the type of protocol.


packages/geoview-core/src/api/config/types/classes/geoview-config/raster-config/wms-config.ts line 54 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

What if === 1... Maybe it is reviewable but I do not see a else after this condition

When there is no question mark ( === 1 means no parameters), the metadataAccessPath is used as is and the query will add the default parameter values to it. That's why there is no else clause.

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan)

@ychoquet ychoquet force-pushed the 2471-Complete_layer_tree_coverage branch from 014ea2e to 1dfb5fd Compare September 12, 2024 20:34
@jolevesq jolevesq merged commit a619207 into Canadian-Geospatial-Platform:develop Sep 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete layer tree coverage
3 participants