-
Notifications
You must be signed in to change notification settings - Fork 42
Replace no more existing "gas-wfs" in example application #466
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
Conversation
sronveaux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chrismayer,
Thanks for tackling this and providing a seemingly stable source for the future.
For sure gas-wfs was a nice example as it had points all over the world and its interactions with the AttributeTable and its dynamic loading could be directly evaluated.
I also liked the one @fschmenger proposed with cities all around the world. However, pinning airports is visually easier and more talkative than pinning cities I think.
Simple question concerning docs/maps-layer-configuration.md, line 156 which looks like this:
| textIcon | Point only, see text. Icons for font
normal 30px Material Iconscan be found here |"textIcon": "local_gas_station"|
Should we still reference the now unused local_gas_station or not ? It can certainly stay as is, this is a valid example. I just wanted to be sure it wasn't "missed"...
I also added another comment on the config file to be discussed fast before merging but I really like it, thanks!
e4438c3 to
7bf8edb
Compare
|
Thanks for your review @sronveaux 👍 I re-added the columnMapping since it was quite messy in the AttributeTable without it. Regarding
I tried this WFS but stumbled over some problems with the service, which I could not fix/correct. But changing to another WFS in a later stage shouldn't be a big deal.
I decided to keep the "old" icon, since it is valid and gives some diversity. I am going to merge once the test suite is green, which fails for |
|
Hi @chrismayer, Perfectly fine like this, as I said, I like the visual with this one ! Concerning the failing unit test, to be completely honest, I experienced it for years at home here where this test was failing if run in headless mode but passed if connecting manually to Since a few months, I have no problems anymore at home but sometimes experience it as you are now when pushing on GitHub. Rerunning the tests once or twice makes it work in the end... I'm thinking about making a fast PR to try increasing timeout value but while experiencing it before at home, this hasn't worked because the request was totally lost... even increasing to 30 seconds didn't work at that time... but we could try 5 seconds instead of 2, perhaps the two problems are not linked at all... |
sronveaux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely fine from my point of view !
Please merge at will !
|
Have fun, it's green now 😉 |
This replaces the no more existing
"gas-wfs"based on the WFS at ows.terrestris.de with another open WFS by German BKG ("Federal Agency for Cartography and Geodesy") delivering airports in Germany:This only affects some example applications.