-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
17.0: Widget map and new map field #2953
base: 17.0
Are you sure you want to change the base?
Conversation
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 @drkpkg. Thanks a lot for sharing. It looks very nice !
Some remarks :
-
blocking point : OCA module still contains leaflet lib. I think we should mutualize. For exemp. for exemple, create a specific module named "web_leafet_lib" and make both module depending on the "lib" module. What do you think ?
-
Question : why use lib version 1.4.0 ? it looks quite old (the latest version is 1.9.4).
-
I think that the name of the field is incorrect. (fields.Map) It doesn't contains a map but a GPS coordinate, AFAIU. So it should be something like fields.Coordinate, (or something similar)
I tried to test it locally.
first error
when trying to display a map widget
float(value.split(",")[0])
AttributeError: 'NoneType' object has no attribute 'split'
Second error
(when trying to display a map widget)
UncaughtPromiseError > OwlError
Uncaught Promise > The following error occurred in onWillStart: "The loading of /map_field/static/lib/leaflet/leaflet.js failed"
I set the PR as draft, feel free to put it as ready, when finished. Thanks !
I think create a new contribution should be better. After that we can document about the new module. This will be a good start to prevent break modules due this change.
Yes, I was working on it and I finished when I read this, sorry! PS: Now leaflet 1.94 is used. |
…aflet_lib, to be used by other modules, like 'web_widget_map'. (see : OCA/web#2953)
Hi @drkpkg. I splitted the a module that contains leaflet lib in two modules here : OCA/geospatial#380
Note : the module provide a parameter to define the tile system to be used. (https://github.com/OCA/geospatial/blob/16.0/web_view_leaflet_map/demo/ir_config_parameter.xml#L14)
|
Dependency updated. Runboat failed, but now the module will have the dependency from web_view_leaflet_map. |
That can not work immediately !
note : the name is not web_view_leaflet_map but web_leaflet_lib. thanks ! |
I did a little PR for leaflet OCA/geospatial#381 |
…aflet_lib, to be used by other modules, like 'web_widget_map'. (see : OCA/web#2953)
…aflet_lib, to be used by other modules, like 'web_widget_map'. (see : OCA/web#2953)
Hi @drkpkg. You can review OCA/geospatial#383 (and then, base your work on this PR ). thanks ! |
zoom: 13, | ||
}; | ||
// eslint-disable-next-line no-undef | ||
this.map = L.tileLayer("https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png", { |
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 could you reuse settings introduced by web_leaflet_lib module ?
See :
values are set here : https://github.com/OCA/geospatial/pull/383/files#diff-5be4d3d480aee272fac8a7719d7470286c43ba57485ca6fc98da0cc374ee8910R11
Thanks !
Previous PR: #2917
I decided do a refactor for this module and let the previous PR stay in closed and dropped.
This new widget is a combination of the
map
field and themap
widget. It allows youto display a map in the form and to select a location by dragging a marker on the map.
I did the pre commit and all was good this time.
The python instance of the field is
fields.Map
.The xml file should look like this:
Note:
Sorry if i duplicate this PR but I think it is the correct way due the refactor.