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

[ADD][12.0] google map drawing #289

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Conversation

brian10048
Copy link
Sponsor Contributor

@gityopie This is to bring your module for Google map drawing into the OCA geospatial repo

@yvaucher @OCA/geospatial-maintainers Comments regarding module name, etc are welcome. It may make more sense to name it base_google_map_drawing since this module doesn't do anything on its own and simply provides a mixin class to be used in other modules.

@gityopie
Copy link

Hi @brian10048
Thanks to bring that module to the OCA Geospatial module
I saw the implementation for the maps drawing is moved to widget instead of view (for me it's fine)

If you need help, don't hesitate to let me know

@yvaucher
Copy link
Member

yvaucher commented Mar 16, 2021

@brian10048 The current module name web_view_google_map_drawing seems fine to me, having a base_ might be confusing as which module depends on which between this one and web_view_google_map

I'm ok to integrate this in this repository. Can you give it some love to the details to make it compliant with OCA Guidelines?

➡️ OCA Guidelines (2018/08/24)

@brian10048 brian10048 force-pushed the add-google-drawing branch 5 times, most recently from 9da09c3 to 2677fc5 Compare March 16, 2021 22:18
@brian10048
Copy link
Sponsor Contributor Author

@yvaucher I agree base_ may be confusing. I renamed it to web_widget_ instead of web_view_ since it is just a new widget for the map view and not a new view.

I have made minor changes to meet OCA guidelines. Let me know if you see any other specific issues with this and I'll be glad to update

Update to use google library parameters for URL
@@ -4,12 +4,13 @@
<t t-set="google_maps_api_key" t-value="request.env['ir.config_parameter'].sudo().get_param('google.api_key_geocode')"/>
<t t-set="google_maps_lang_localization" t-value="request.env['ir.config_parameter'].sudo().get_param('google.lang_localization')"/>
<t t-set="google_maps_region_localization" t-value="request.env['ir.config_parameter'].sudo().get_param('google.region_localization')"/>
<t t-set="google_maps_libraries" t-value="request.env['ir.config_parameter'].sudo().get_param('google.maps_libraries')"/>

This comment was marked as resolved.

Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

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

LGTM

@yvaucher
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-289-by-yvaucher-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 2f69c15 into OCA:12.0 Mar 23, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4ee44c0. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants