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

[Datahub] Added dynamic legend generation based on map context #1069

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

ronitjadhav
Copy link
Collaborator

@ronitjadhav ronitjadhav commented Dec 18, 2024

Description

This PR integrates the @geospatial-sdk/legend npm package to dynamically generate map legends based on the map context in the MapViewComponent. The legend generation works for WMTS and WMS services.

Architectural Changes

  1. Integration of @geospatial-sdk/legend npm package:

    • The @geospatial-sdk/legend package is used to dynamically generate map legends based on the map context.
  2. Creation of MapLegendComponent:

    • Created MapLegendComponent to handle the generation and display of map legends.
  3. Modification of MapViewComponent:

    • Added MapLegendComponent to the MapViewComponent.
    • Implemented methods toggleLegend and onLegendStatusChange to manage the display and status of the legend.

Screenshots

image

image

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Affected libs: feature-record, feature-router, ui-map, feature-dataviz, feature-editor, feature-map, database-dump, data-access-datafeeder, api-metadata-converter, api-repository, feature-catalog, feature-search, feature-auth, ui-search, feature-notifications, util-data-fetcher, common-fixtures, util-shared, ui-elements, ui-catalog, ui-widgets, ui-inputs, ui-layout, data-access-gn4, util-app-config, common-domain, ui-dataviz, util-i18n,
Affected apps: metadata-editor, datahub, demo, webcomponents, datafeeder, map-viewer, search, metadata-converter, data-platform,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

Copy link
Contributor

github-actions bot commented Dec 18, 2024

📷 Screenshots are here!

@ronitjadhav ronitjadhav requested a review from jahow December 18, 2024 13:53
@fgravin
Copy link
Member

fgravin commented Dec 18, 2024

What happens if you click on a feature, it use to display the attribute in a popup at the same place.

package.json Show resolved Hide resolved
@ronitjadhav
Copy link
Collaborator Author

@fgravin The legend moves to the side if the attribute table is active.

image

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thank you! I did a first review without testing, let me know if you need any more information!

package.json Show resolved Hide resolved
<gn-ui-button
type="primary"
(buttonClick)="toggleLegend()"
extraClass="rounded p-2 text-xs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably don't need the rounded p-2 classes here; take a look at this component for a way to have a nice and square close button:

<gn-ui-button
type="light"
style="
--gn-ui-button-padding: 0px;
--gn-ui-button-width: 24px;
--gn-ui-button-height: 24px;
"
class="ml-[8px] mr-[10px]"
(buttonClick)="removeItem(index)"
data-test="remove-item"
><span class="material-symbols-outlined gn-ui-icon-medium">close</span>
</gn-ui-button>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jahow Does the close button look better now?

image

</gn-ui-button>
</div>
<gn-ui-map-legend
[context]="mapContext$ | async"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test is failing because this adds another subscription to mapContext$, which triggers the extent computation one more time; in the tests we only resolve the extent computation promise once, so the other does not complete and as such one of the subscriptions doesn't receive the full computed context.

Long story short, I think a good fix is to add this:

    withLatestFrom(this.mdViewFacade.metadata$),
    map(([context, metadata]) => {
      if (context.view) return context
      const extent = this.mapUtils.getRecordExtent(metadata)
      const view = extent ? { extent } : null
      return {
        ...context,
        view,
      }
    }),
+   shareReplay(1) // line 172
  )

Then we won't do unnecessary extent computations :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanksa a lot :)

@coveralls
Copy link

Coverage Status

coverage: 84.621% (-1.2%) from 85.834%
when pulling 692832e on map-legend
into da5f436 on main.

@ronitjadhav ronitjadhav merged commit 67e23df into main Dec 19, 2024
14 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.

4 participants