Skip to content

PB-414: marker overlapping title#806

Merged
LukasJoss merged 18 commits intodevelopfrom
feat-PB-414-marker-overlapping-title
May 3, 2024
Merged

PB-414: marker overlapping title#806
LukasJoss merged 18 commits intodevelopfrom
feat-PB-414-marker-overlapping-title

Conversation

@LukasJoss
Copy link
Contributor

@LukasJoss LukasJoss commented Apr 25, 2024 •
edited by github-actions bot
Loading

@LukasJoss
Copy link
Contributor Author

LukasJoss commented Apr 25, 2024 •
edited
Loading

Exaple kml:
https://sys-s.dev.bgdi.ch/mr7dwvf1zanw
https://sys-s.dev.bgdi.ch/9wyu8ahpjpzh
I have opted now for a fixed offset because kml does not support text offsets (since a fixed offset does not make much sense in 3d space) similar to how it seems to be implemented on google earth (but 2d)

@cypress
Copy link

cypress bot commented Apr 25, 2024 •
edited
Loading

Passing run #2018 ↗︎

0 160 20 0 Flakiness 0

Details:

PB-414: Add end to end test to check kml title offset
Project: web-mapviewer Commit: 0bdc6b973d
Status: Passed Duration: 04:30 💡
Started: May 2, 2024 12:47 PM Ended: May 2, 2024 12:52 PM

Review all test suite changes for PR #806 ↗︎

@LukasJoss LukasJoss force-pushed the feat-PB-414-marker-overlapping-title branch 5 times, most recently from d3f033c to 278ebf5 Compare April 29, 2024 12:00
@LukasJoss LukasJoss requested review from ltkum, ltshb and pakb April 29, 2024 12:10
@LukasJoss LukasJoss marked this pull request as ready for review April 29, 2024 12:35
ltshb
ltshb reviewed Apr 29, 2024
View reviewed changes
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.

Personally I prefer the text to be below the marker, especially for the position marker having the text below is more intuitive.

@LukasJoss
Copy link
Contributor Author

LukasJoss commented Apr 29, 2024

Personally I prefer the text to be below the marker, especially for the position marker having the text below is more intuitive.

We can discuss that tomorrow, maybe somebody else has a strong opinion about it.

@ltshb ltshb changed the title Feat pb 414 marker overlapping title PB-414: marker overlapping title Apr 30, 2024
@LukasJoss LukasJoss force-pushed the feat-PB-414-marker-overlapping-title branch 2 times, most recently from 8e991fa to db45ad0 Compare April 30, 2024 13:33
@LukasJoss LukasJoss requested a review from ltshb April 30, 2024 13:44
ltshb
ltshb reviewed Apr 30, 2024
View reviewed changes
@LukasJoss LukasJoss force-pushed the feat-PB-414-marker-overlapping-title branch from 357dcb2 to 4c44e28 Compare May 1, 2024 06:52
@LukasJoss
Copy link
Contributor Author

LukasJoss commented May 1, 2024 •
edited
Loading

@ltshb currently when exporting and then reimporting a kml file, there will be no offset since it is considered to be a legacy layer as there is no metedata when importing it from a file

web-mapviewer/src/api/layers/KMLLayer.class.js

Lines 89 to 91 in 4c44e28

isLegacy() {
return this.kmlMetadata?.author !== 'web-mapviewer'
}
should there some metadata be added in the kml file itself that we produce so we can detect that?
we could also detect that it fetches the icons from map.geo.admin instead of api3.geo.admin

@LukasJoss LukasJoss requested a review from ltshb May 1, 2024 07:29
pakb
pakb approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

@LukasJoss LukasJoss force-pushed the feat-PB-414-marker-overlapping-title branch 5 times, most recently from 9e7f847 to d2f211d Compare May 1, 2024 12:50
ltshb
ltshb approved these changes May 2, 2024
View reviewed changes
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.

Looks very thanks a lot.
Now that the offset is in KML it might make sense to test the presence/absence of the offset in KML in e2e test, this should be easily done ?

LukasJoss added 17 commits May 2, 2024 14:43
@LukasJoss
PB-414: Add offset calculation to kml to feature calculation
070b672
@LukasJoss
PB-414: Add offset calculation in drawing mode
d08dc33
@LukasJoss
PB-414: Initialize correct default value for offset
36d6943
@LukasJoss
PB-414: Update variable names
eef525e
@LukasJoss
PB-414: Read anchor values instead of using default values
bc384c8
@LukasJoss
PB-414: Fix cypress tests
0bf48e7
@LukasJoss
PB-414: Call offset calculation only if it is a marker
63da662
@LukasJoss
PB-414: Update text offset calculation
3626ee5
@LukasJoss
PB-414: Update constructor default value
a14d827
@LukasJoss
PB-414: Implement changes from PR
b2712e1
@LukasJoss
PB-414: Name not necessary for offset due to transparent boundary of …
d930213 
…icon
@LukasJoss
PB-414: Add icon size (2D) as parameter
23b37b8
@LukasJoss
PB-414: Add kml layer as parameter so as to not change offset of lega…
f6160af 
…cy drawings
@LukasJoss
PB-414: Fix failing e2e test
623aa83
@LukasJoss
PB-414: Implement changes from PR
cef2074
@LukasJoss
PB-414: Add offset as extended data for marker features
e56f41b
@LukasJoss
PB-414: Add end to end test to check kml title offset
0bdc6b9
@LukasJoss LukasJoss force-pushed the feat-PB-414-marker-overlapping-title branch from d2f211d to 0bdc6b9 Compare May 2, 2024 12:43
@LukasJoss
Copy link
Contributor Author

LukasJoss commented May 2, 2024 •
edited
Loading

Looks very thanks a lot. Now that the offset is in KML it might make sense to test the presence/absence of the offset in KML in e2e test, this should be easily done ?

I assume it suffices to test the parsing from kml to feature and not the one from feature to kml
https://github.com/geoadmin/web-mapviewer/blob/0bdc6b973db7c5000743c14e7b9d24a2c520bdf0/src/utils/tests/kmlUtils.spec.js#L121-L145

@LukasJoss LukasJoss merged commit 5ca36a3 into develop May 3, 2024
@LukasJoss LukasJoss deleted the feat-PB-414-marker-overlapping-title branch May 3, 2024 07:19
@cypress cypress bot mentioned this pull request May 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pakb pakb pakb approved these changes

@ltshb ltshb ltshb approved these changes

@ltkum ltkum Awaiting requested review from ltkum

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LukasJoss @pakb @ltshb