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

2321-draft version aoi #2325

Conversation

guichoquette
Copy link
Contributor

@guichoquette guichoquette commented Jun 27, 2024

Description

Created the Area of interest package. The first version display the AOI icon in the app bar but the panel used is Data-Table Panel.

Fixes # (2321)

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?

With a local host I was loading the AOI page and making sure the icon was in the side panel and the page title was Area of interest. I still have a little problem, when I over above the AOI icon it says "aoiPanel.title".

URL deploy : https://guichoquette.github.io/geoview/area-of-interest.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

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 10 of 17 files at r1, all commit messages.
Reviewable status: 10 of 17 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


.vscode/settings.json line 79 at r1 (raw file):

    }
  ],
  "editor.guides.indentation": false

Why this setting?


packages/geoview-core/public/templates/area-of-interest.html line 4 at r1 (raw file):

<html lang="en">

<head>

Do not use a template, just add to configuration navigator


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 120 at r1 (raw file):

      legend: { icon: <HubOutlinedIcon />, content: <Legend fullWidth containerType={CONTAINER_TYPE.APP_BAR} /> },
      layers: { icon: <LayersOutlinedIcon />, content: <LayersPanel containerType={CONTAINER_TYPE.APP_BAR} /> },
      'aoi-panel': { icon: <AoiIcon />, content: <Datapanel containerType={CONTAINER_TYPE.APP_BAR} /> },

basemap panel is not here... should be injected automatically by plugin loading

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.

Reviewable status: 10 of 17 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r1 (raw file):

{
  "isOpen": false,
  "canSwichProjection": false,

We should clean this schema, just keep isOpen


packages/geoview-aoi-panel/schema.json line 8 at r1 (raw file):

  "comments": "Configuration for GeoView basemap panel package.",
  "additionalProperties": false,
  "definitions": {

Clean this file, just keep isOpen


packages/geoview-aoi-panel/src/area-of-interest-style.ts line 6 at r1 (raw file):

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const getSxClasses = (theme): any => ({
  basemapCard: {

You can rename basemap to aoi for class name. Style should be pretty similar

Copy link
Contributor Author

@guichoquette guichoquette 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: 10 of 17 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


.vscode/settings.json line 79 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Why this setting?

Done. Those were personal settings that I accidentally imported


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

We should clean this schema, just keep isOpen

Done.


packages/geoview-aoi-panel/schema.json line 8 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Clean this file, just keep isOpen

Done.


packages/geoview-aoi-panel/src/area-of-interest-style.ts line 6 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

You can rename basemap to aoi for class name. Style should be pretty similar

Done.


packages/geoview-core/public/templates/area-of-interest.html line 4 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Do not use a template, just add to configuration navigator

Done.

@guichoquette guichoquette force-pushed the 2321-Draft-version-AOI branch from f5510b3 to 77e2494 Compare July 2, 2024 02:43
Copy link
Contributor Author

@guichoquette guichoquette 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: 7 of 21 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 120 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

basemap panel is not here... should be injected automatically by plugin loading

Done.

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 17 files at r1, 12 of 13 files at r2, all commit messages.
Reviewable status: 20 of 21 files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 39 at r2 (raw file):

      AoiPanel: {
        title: 'Area of Interest',
        info: {

For translation, you can keep just the title


packages/geoview-core/public/templates/loading-packages.html line 123 at r2 (raw file):

          cgpv.api.plugin.removePlugin('basemap-panel', 'mapWM');
        });
        

Remove white spaces


packages/geoview-core/public/templates/loading-packages.html line 141 at r2 (raw file):

          cgpv.api.plugin.removePlugin('aoi-panel', 'mapWM');
        });
        

Remove white spaces


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 120 at r1 (raw file):

Previously, guichoquette wrote…

Done.

Remove aoi-panel from app-bar component. Plugins are note reference inside default app-bar core component

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.

Reviewable status: 20 of 21 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/public/templates/demos-navigator.html line 125 at r2 (raw file):

            Basemap LCC Simple-Labeled (overview map hide on zoom 7 and lower)
          </option>
          <option value="./configs/navigator/26-area-of-iterest">Area of Interest</option>

Put it just before package base map panel and name it Package Area of Interest

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.

Reviewable status: 20 of 21 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/public/templates/demos-navigator.html line 125 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Put it just before package base map panel and name it Package Area of Interest

Plus missing the .json extension and typo int he name iterest instead of interest

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.

Reviewable status: 20 of 21 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 276 at r2 (raw file):

          .then((constructor: AbstractPlugin | ((pluginId: string, props: TypeJsonObject) => TypeJsonValue)) => {
            Plugin.addPlugin(
              'basemap-panel',

This should not be hard coded I think

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.

Reviewable status: 20 of 21 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/src/api/config/types/config-constants.ts line 221 at r2 (raw file):

 * Definition of the default order of the tabs inside appbar
 */
export const CV_DEFAULT_APPBAR_TABS_ORDER = ['geolocator', 'legend', 'layers', 'details', 'data-table', 'guide', 'aoi-panel'];

Need to remove here as aoi is not a default app bar component

@guichoquette guichoquette force-pushed the 2321-Draft-version-AOI branch from 77e2494 to ccf004f Compare July 2, 2024 18:45
Copy link
Contributor Author

@guichoquette guichoquette 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: 15 of 22 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 39 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

For translation, you can keep just the title

Done.


packages/geoview-core/public/templates/demos-navigator.html line 125 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Plus missing the .json extension and typo int he name iterest instead of interest

Done.


packages/geoview-core/public/templates/loading-packages.html line 123 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Remove white spaces

Done.


packages/geoview-core/public/templates/loading-packages.html line 141 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Remove white spaces

Done.


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 120 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Remove aoi-panel from app-bar component. Plugins are note reference inside default app-bar core component

Done.


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 276 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

This should not be hard coded I think

Done.


packages/geoview-core/src/api/config/types/config-constants.ts line 221 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Need to remove here as aoi is not a default app bar component

Done.

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 7 files at r3, all commit messages.
Reviewable status: 16 of 22 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 39 at r2 (raw file):

Previously, guichoquette wrote…

Done.

I meant
en: { title: 'Area of Interest}, fr: { title: "Région d'intérêt' }

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 7 files at r3.
Reviewable status: 17 of 22 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/public/templates/demos-navigator.html line 131 at r3 (raw file):

          <option value="./configs/navigator/07-basic-appbar.json">Basic map with app bar</option>
          <option value="./configs/navigator/08-package-basemap.json">Package basemap panel</option>
          <option value="./configs/navigator/26-package-area-of-interest.json">Area of Interest</option>

Should be "Package Area of interest", put it before the base map one... not in the middle of 2 basemap package demo

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 3 of 7 files at r3.
Reviewable status: 20 of 22 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

            ).catch((error) => {
              // Log
              logger.logPromiseFailed(`api.plugin.addPlugin in useEffect in ${pluginName}`, error);

in app-bar for ${pluginName}

@guichoquette guichoquette force-pushed the 2321-Draft-version-AOI branch from ccf004f to 4a10e4f Compare July 2, 2024 20:22
Copy link
Contributor Author

@guichoquette guichoquette 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: 12 of 22 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 39 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

I meant
en: { title: 'Area of Interest}, fr: { title: "Région d'intérêt' }

Done.


packages/geoview-core/public/templates/demos-navigator.html line 131 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Should be "Package Area of interest", put it before the base map one... not in the middle of 2 basemap package demo

Done.


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

in app-bar for ${pluginName}

Done.

@jolevesq jolevesq marked this pull request as ready for review July 3, 2024 13:27
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 2 of 10 files at r4, all commit messages.
Reviewable status: 14 of 22 files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

          label: {
            name: 'with labels',
          },

You can remove all this for en and fr

Code quote:

        info: {
          transport: {
            name: 'Transport',
            description: `The Canada Base Map - Transportation (CBMT). This web mapping service provides spatial reference context with an emphasis on transportation networks.
                          It is designed especially for use as a background map in a web mapping application or geographic information system (GIS).`,
          },
          simple: {
            name: 'Simple',
          },
          shaded: {
            name: 'Shaded relief',
            description: `The Canada Base Map - Elevation (CBME) web mapping services of the Earth Sciences Sector at Natural Resources Canada,
                          is intended primarily for online mapping application users and developers`,
          },
          osm: {
            name: 'Open Street Maps',
          },
          nogeom: {
            name: 'No geometry',
          },
          label: {
            name: 'with labels',
          },

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 5 of 10 files at r4.
Reviewable status: 19 of 22 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

Previously, guichoquette wrote…

Done.

api.plugin.addPlugin in useEffect in app-bar for ${pluginName}

If we remove the app-bar in message we will not know were the error came from

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 10 files at r4.
Reviewable status: 20 of 22 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @guichoquette, and @ychoquet)

Copy link
Member

@Alex-NRCan Alex-NRCan left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 17 files at r1, 5 of 13 files at r2, 1 of 7 files at r3, 9 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DamonU2, @guichoquette, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 126 at r4 (raw file):

  override onRemoved(): void {
    // reset AOI array
    this.mapViewer().basemap.basemaps = [];

We should call a function on the Basemap object (e.g.: this.mapViewer().basemap.clearBasemaps() which clears the basemaps list. That way, if there are other components working on the basemaps array, we can make them aware of this clearing happening. (Or we use an event-processor/state thing, can let Johann decide what's best here).


packages/geoview-core/src/api/config/types/map-schema-types.ts line 59 at r4 (raw file):

/** Supported app bar values. */
export type TypeValidAppBarCoreProps =

BTW.
Maybe it'd be cool to reuse that type (or some kind of similar logic) in the Plugin.loadPlugin, Plugin.loadScript function signatures?


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 271 at r4 (raw file):

    logger.logTraceUseEffect('APP-BAR - appBarConfig');

    const processPlugin = (pluginName: string): void => {

Minor.
Should pluginName be typed 'TypeValidAppBarCorePropsin the signature here and the 'as TypeValidAppBarCoreProps' be put at a higher level, whenprocessPlugin()` is called? It'd be a bit more straightforward?

Copy link
Member

@Alex-NRCan Alex-NRCan 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: all files reviewed, 6 unresolved discussions (waiting on @DamonU2, @guichoquette, @jolevesq, and @ychoquet)


packages/geoview-core/src/api/config/types/map-schema-types.ts line 59 at r4 (raw file):

Previously, Alex-NRCan (Alex) wrote…

BTW.
Maybe it'd be cool to reuse that type (or some kind of similar logic) in the Plugin.loadPlugin, Plugin.loadScript function signatures?

On second thought, maybe we don't, because we want the loadScript/loadPlugin to be easily extensible by 3rd party developers, so working with a string might be easier to extend. To be determined.

Copy link
Contributor

@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.

Reviewed 5 of 17 files at r1, 6 of 13 files at r2, 1 of 7 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DamonU2, @guichoquette, and @jolevesq)

Copy link
Contributor Author

@guichoquette guichoquette 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: all files reviewed, 6 unresolved discussions (waiting on @DamonU2 and @jolevesq)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

You can remove all this for en and fr

Done.


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

api.plugin.addPlugin in useEffect in app-bar for ${pluginName}

If we remove the app-bar in message we will not know were the error came from

Done.

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.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DamonU2 and @guichoquette)


packages/geoview-aoi-panel/src/index.tsx line 126 at r4 (raw file):

Previously, Alex-NRCan (Alex) wrote…

We should call a function on the Basemap object (e.g.: this.mapViewer().basemap.clearBasemaps() which clears the basemaps list. That way, if there are other components working on the basemaps array, we can make them aware of this clearing happening. (Or we use an event-processor/state thing, can let Johann decide what's best here).

In the viewer there is the default basemap array... May be a good thing to have a centralized way of dealing with this this. I will create an issue.

@guichoquette This line can be remove as it is not part of aoi plugin

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.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DamonU2 and @guichoquette)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

Previously, guichoquette wrote…

Done.

Not done, object is still there

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.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @guichoquette)


packages/geoview-core/src/api/config/types/map-schema-types.ts line 59 at r4 (raw file):

Previously, Alex-NRCan (Alex) wrote…

On second thought, maybe we don't, because we want the loadScript/loadPlugin to be easily extensible by 3rd party developers, so working with a string might be easier to extend. To be determined.

I think we are right, if we do type, it will be more complicated for 3rd party

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.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @guichoquette)


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

Previously, guichoquette wrote…

Done.

Not done, line should be : logger.logPromiseFailed(api.plugin.addPlugin in useEffect in app-bar for ${pluginName}, error);


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 271 at r4 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Minor.
Should pluginName be typed 'TypeValidAppBarCorePropsin the signature here and the 'as TypeValidAppBarCoreProps' be put at a higher level, whenprocessPlugin()` is called? It'd be a bit more straightforward?

Good comment, @guichoquette you can apply

Copy link
Member

@DamonU2 DamonU2 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 17 files at r1, 6 of 13 files at r2, 1 of 7 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Alex-NRCan, @guichoquette, and @jolevesq)


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r4 (raw file):

{
  "isOpen": false
}

Add blank end line - if you set up prettier to auto format on save, I think it will automatically add this


packages/geoview-aoi-panel/package.json line 40 at r4 (raw file):

    "@types/react": "^18.2.0"
  }
}

Same here, add new line


packages/geoview-aoi-panel/README.md line 3 at r4 (raw file):

# geoview-Area-of-interest

A package that allows to display a collection of areas to visit on the map.

"A package to display..." or "A package that allows a user to display..."


packages/geoview-aoi-panel/schema.json line 25 at r4 (raw file):

    "isOpen"
  ]
}

New line at end

Copy link
Contributor Author

@guichoquette guichoquette 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: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Not done, object is still there

Done.


packages/geoview-aoi-panel/src/index.tsx line 126 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

In the viewer there is the default basemap array... May be a good thing to have a centralized way of dealing with this this. I will create an issue.

@guichoquette This line can be remove as it is not part of aoi plugin

Do I only remove the line or the entire method? If it's only the line the comment is Done.


packages/geoview-core/src/api/config/types/map-schema-types.ts line 59 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

I think we are right, if we do type, it will be more complicated for 3rd party

I'm not sure if I have something to do here? Can I get more specifications if it's the case?


packages/geoview-core/src/core/components/app-bar/app-bar.tsx line 282 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Not done, line should be : logger.logPromiseFailed(api.plugin.addPlugin in useEffect in app-bar for ${pluginName}, error);

Done.

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.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan and @guichoquette)


packages/geoview-core/src/api/config/types/map-schema-types.ts line 59 at r4 (raw file):

Previously, guichoquette wrote…

I'm not sure if I have something to do here? Can I get more specifications if it's the case?

Nothing to do.

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.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan and @guichoquette)


packages/geoview-aoi-panel/src/index.tsx line 126 at r4 (raw file):

Previously, guichoquette wrote…

Do I only remove the line or the entire method? If it's only the line the comment is Done.

Yes, you can remove the line

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.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan and @guichoquette)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

Previously, guichoquette wrote…

Done.

We will discuss as it is still there, cleanup of old basemap translation is not removed

Copy link
Contributor Author

@guichoquette guichoquette 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: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r4 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

Add blank end line - if you set up prettier to auto format on save, I think it will automatically add this

Done.


packages/geoview-aoi-panel/package.json line 40 at r4 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

Same here, add new line

Done.


packages/geoview-aoi-panel/README.md line 3 at r4 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

"A package to display..." or "A package that allows a user to display..."

Done.


packages/geoview-aoi-panel/schema.json line 25 at r4 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

New line at end

Done.

@guichoquette guichoquette force-pushed the 2321-Draft-version-AOI branch from 4a10e4f to 3763d7c Compare July 3, 2024 15:22
Copy link
Contributor Author

@guichoquette guichoquette 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: 15 of 22 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/src/index.tsx line 61 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

We will discuss as it is still there, cleanup of old basemap translation is not removed

Done.


packages/geoview-aoi-panel/src/index.tsx line 126 at r4 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Yes, you can remove the line

Done.

Copy link
Member

@DamonU2 DamonU2 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: 15 of 22 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan, @guichoquette, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r4 (raw file):

Previously, guichoquette wrote…

Done.

Still not there. The file should be 4 lines, with the last one blank, same with the other files with this issue

@guichoquette guichoquette force-pushed the 2321-Draft-version-AOI branch from 3763d7c to cfba8dc Compare July 3, 2024 16:23
Copy link
Contributor Author

@guichoquette guichoquette 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: 15 of 23 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @ychoquet)


packages/geoview-aoi-panel/default-config-area-of-interest.json line 3 at r4 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

Still not there. The file should be 4 lines, with the last one blank, same with the other files with this issue

Done.

Copy link
Member

@DamonU2 DamonU2 left a comment

Choose a reason for hiding this comment

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

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

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 7 of 7 files at r5, 5 of 5 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)

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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)

@jolevesq jolevesq merged commit 9019eb7 into Canadian-Geospatial-Platform:develop Jul 3, 2024
5 of 6 checks passed
@guichoquette guichoquette deleted the 2321-Draft-version-AOI branch July 19, 2024 18:19
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.

5 participants