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

Use geospatial-sdk for maps #949

Merged
merged 12 commits into from
Sep 18, 2024
Merged

Use geospatial-sdk for maps #949

merged 12 commits into from
Sep 18, 2024

Conversation

jahow
Copy link
Collaborator

@jahow jahow commented Jul 24, 2024

Description

This PR refactors significantly the map-related logic of GeoNetwork-UI to rely mostly on the geospatial-sdk library.

How things work now:

  • The gn-ui-map-container component is used to display a map; it only takes a map context (MapContext type from geospatial-sdk) as an input, and offers the following outputs: featuresClicked, featuresHovered, mapClick
    • this component will add basemap layers to the received context, as well as apply view constraints (max zoom, max extent...)
    • the basemap layers and view constraints can be changed by using InjectionTokens
    • this components provides an openlayersMap promise that resolves to the OL map when it's ready, if necessary
  • The gn-ui-map-state-container component is connected to the map state, accessible through the MapFacade; this facade lets us provide a new context to the map, and stores currently selected features

Depending on the use case, a map can be shown with or without using the state; both ways are valid.

A lot of code was removed and brought back into the geospatial-sdk: map and layer creation, extent computation, etc.

Important: these features are not working yet and can be considered regressions:

  • When selecting a vector feature on a map, it doesn't appear in a different color
  • When zooming in on a feature, there is currently no animation

Architectural changes

The following classes are gone:

  • feature info service
  • map instance
  • map manager
  • map container
  • map context

Those features are now handled by the geospatial-sdk.

ogc-client was updated.

Several components were made standalone in the process. The UiMap module is gone entirely because all of its components are now standalone!

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 Jul 24, 2024

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

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

@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch 2 times, most recently from e73d854 to 0d95f46 Compare September 13, 2024 16:15
@jahow jahow changed the title [WIP] Use geospatial-sdk for maps Use geospatial-sdk for maps Sep 13, 2024
@jahow jahow marked this pull request as ready for review September 13, 2024 16:15
@jahow
Copy link
Collaborator Author

jahow commented Sep 13, 2024

Most of the features should work; still working on the missing tests

@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch from 0d95f46 to 2f3fe49 Compare September 15, 2024 14:04
Copy link
Contributor

github-actions bot commented Sep 15, 2024

📷 Screenshots are here!

@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch 2 times, most recently from 3712499 to 15f8c8e Compare September 15, 2024 16:35
@coveralls
Copy link

coveralls commented Sep 15, 2024

Coverage Status

coverage: 83.858% (+0.07%) from 83.787%
when pulling dd44d80 on use-geoespatial-sdk-for-maps
into 86d11dd on main.

@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch 3 times, most recently from 59d28c0 to f58a017 Compare September 16, 2024 09:49
Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Waooo, thanks for the huge work @jahow ! It looks very clean and the geospatial-sdk seems handy to use. Maybe some words on the possibilities how to use map (with and without a map state) could be useful in the gn-ui doc?

When testing, most things worked smoothly, the only things I've noticed were:

I didn't test the datafeeder. Not sure if the failing CI is currently expected.

@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch 2 times, most recently from 7619966 to 4a38d07 Compare September 18, 2024 09:23
@jahow
Copy link
Collaborator Author

jahow commented Sep 18, 2024

Added a bit of doc :) thanks for the review @tkohr!

The gn-mapviewer web component should work now
layers should appear with their label in the layers panel
Add missing styles in web components
Fix a failing e2e test
@jahow jahow force-pushed the use-geoespatial-sdk-for-maps branch from 4a38d07 to dd44d80 Compare September 18, 2024 09:34
@jahow jahow merged commit 4992149 into main Sep 18, 2024
13 checks passed
@jahow jahow deleted the use-geoespatial-sdk-for-maps branch September 18, 2024 11:14
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.

3 participants