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

PB-856 : add a style selector for KML in layer settings #1047

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pakb
Copy link
Contributor

@pakb pakb commented Aug 30, 2024

this enables users to opt-out of the default style (Google Earth like) we apply by default to external KML, and let them go back to the GeoAdmin (all red) style that was previously (on mf-geoadmin3) applied across the board.

adding this choice to the layer URL extra attributes

Test link

Copy link

cypress bot commented Aug 30, 2024

web-mapviewer    Run #3749

Run Properties:  status check failed Failed #3749  •  git commit 6352abed42: PB-856 : add a style selector for KML in layer settings
Project web-mapviewer
Branch Review feat-PB-856-add-kml-style-selector
Run status status check failed Failed #3749
Run duration 04m 45s
Commit git commit 6352abed42: PB-856 : add a style selector for KML in layer settings
Committer Pascal Barth
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 210
View all changes introduced in this branch ↗︎

Tests for review

Failed  tests/cypress/tests-e2e/importToolFile.cy.js • 1 failed test • e2e/electron/mobile

View Output

Test Artifacts
The Import File Tool > Import KML file error handling Test Replay Screenshots

@pakb pakb force-pushed the feat-PB-856-add-kml-style-selector branch 4 times, most recently from 0787d65 to 185ba67 Compare September 11, 2024 14:50
@pakb pakb requested review from ltshb and ltkum September 11, 2024 15:22
@pakb pakb force-pushed the feat-PB-856-add-kml-style-selector branch from 185ba67 to 4b0ac63 Compare September 13, 2024 09:21
@pakb pakb requested a review from ltshb September 13, 2024 12:37
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

When opening the drawing menu and clicking on add icon, the icon is missing and you have the following type error
image

@ltshb
Copy link
Contributor

ltshb commented Sep 16, 2024

@pakb @hansmannj With styling and icons we might have a general issue with the introduction of babs icons, this PR (kml style selector) and PB-64 self-contained KML. I did not yet check and tested the babs icon for legacy icons, but did you think about this ? What happen if you open and edit a drawing with legacy babs icon ?
Currently (at least before babs icon implementation) the icon selector was a mapping between icon url from the drawing and our icons service which only work if the url was found in our service, now with legacy unlisted icons what do we do ?
The same question will also apply for self-contain kml where the icon might not be found in our service-icons if we deprecated them, what do we do in this case ?
The response of these questions might impact the style selector, we should have a discussion about this tomorrow.

@pakb pakb force-pushed the feat-PB-856-add-kml-style-selector branch from 4b0ac63 to 33725a6 Compare October 2, 2024 08:23
@pakb pakb requested a review from ismailsunni October 2, 2024 08:25
@ismailsunni
Copy link
Contributor

I tried to load Rossier.kml from the JIRA ticket, but it doesn't allow me anymore. It works in production
image

@pakb pakb force-pushed the feat-PB-856-add-kml-style-selector branch from 33725a6 to 4cb37d2 Compare November 1, 2024 16:30
@hansmannj
Copy link
Member

@pakb @ismailsunni @ltshb : What's the status here?
I think the issues with the BABS icons should be solved. We keep on providing the old icons but unlist them in the viewer. What else needs to be clarified for this PR to be merged?

@ismailsunni
Copy link
Contributor

using this PR test link:

  • I no longer able to load KML file using the import file tool, but I can still load it by using drag and drop

this enables users to opt-out of the default style (Google Earth like) we apply by default to external KML, and let them go back to the GeoAdmin (all red) style that was previously (on mf-geoadmin3) applied across the board.

adding this choice to the layer URL extra attributes
@pakb pakb force-pushed the feat-PB-856-add-kml-style-selector branch from 4cb37d2 to 6352abe Compare November 5, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants