Contact Info & Map Widget: Remove Geocoding and add map to form #12474

kbrown9 · 2019-05-28T00:54:06Z

Fixes #10355
Fixes #12925
Fixes #12913

Changes proposed in this Pull Request:

Remove the use of the Google Maps Geocoding API. The Geocoding API is only used to verify that the address is valid. After these changes are made, users will need to enable only one Google Maps API (Maps Embed API).
Add the Google Maps embedded map to the widget's admin form. This change will allow users to view the maps that will be shown on their site in the admin form. The users will be able to verify that the maps are correct and receive a useful error if the API key is invalid.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This change adds a feature to the existing Contact Info & Map widget.

Testing instructions:

Add a new Contact Info & Maps widget (Appearance -> Widgets -> Contact Info & Map), or access an existing Contact Info & Maps widget.
Check the Show Map checkbox to show the API key box and the embedded map.
The embedded map iframe will show this error when an API key is not entered:

Google Maps Platform rejected your request. You must use an API key to authenticate each request to Google Maps Platform APIs. For additional information, please refer to http://g.co/dev/maps-no-account
Enter an invalid API key. The embedded map iframe will show this error when an invalid API key is entered:

Google Maps Platform rejected your request. The provided API key is invalid.
Enter a valid Google Maps API key. (The Embed Maps API must be enabled for the API key.) Save the widget, and the embedded map will be shown in the admin form.

Proposed changelog entry for your changes:

Contact Info Widget: Remove use of the Geocoding API and add an embedded map to the admin form

jetpackbot · 2019-05-28T00:56:46Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against 04cf9ac

kbrown9 · 2019-05-28T16:56:45Z

I just noticed that the Customize page will show inconsistent maps when an address is changed. The preview page will show the map for the new address. However, the map in the widget's admin form doesn't update, so it shows the map for the old address.

I'll try to think of a solution for this.

kbrown9 · 2019-06-04T17:06:05Z

Following-up on my last comment - I addressed the issue with inconsistent maps on the Customizer page in the latest commit. In this commit, the embedded map is not displayed in the widget admin form on the Customizer page. Since the Customizer preview displays the embedded map, it's not necessary to display the map in the widget admin form.

I used the is_customize_preview() function (code reference) to detect whether the widget admin form is being displayed on the Customizer page.

jeherve

Excellent work! I'd like to suggest a few changes, and to make things easier I've just submitted this PR to your branch:
kbrown9#1

If you are happy with those changes, you can merge the PR and my changes will be automatically added to your PR here.

Beyond those changes, I think we should take that opportunity to better display error messages in the customizer when something is wrong.
Those could be displayed in the widget settings directly, just like on the old settings screen. They could also be displayed in the widget preview directly, since that's where site owners will check what the map will look like.

Let me know what you think.

kbrown9 · 2019-06-06T18:30:55Z

Excellent work! I'd like to suggest a few changes, and to make things easier I've just submitted this PR to your branch:
kbrown9#1

If you are happy with those changes, you can merge the PR and my changes will be automatically added to your PR here.

Beyond those changes, I think we should take that opportunity to better display error messages in the customizer when something is wrong.
Those could be displayed in the widget settings directly, just like on the old settings screen. They could also be displayed in the widget preview directly, since that's where site owners will check what the map will look like.

Let me know what you think.

Thank you for this review! I think all of these changes look great.

I'll work on displaying the error message in the customizer!

jeherve · 2019-06-20T13:08:09Z

@kbrown9 Is this ready for another review?

kbrown9 · 2019-06-20T18:59:43Z

@kbrown9 Is this ready for another review?

@jeherve Yes, it's ready now! I just pushed my last commit. So, I've added three new commits on top of your changes:

commit 7f4b2e09: This commit includes changes that I think improve readability.
commit 43cb2133: This commit displays API key errors in the Contact Info widgets on the Customizer preview page.
commit 15910c91: This commit displays API key errors in the Contact Info admin forms on the Customizer preview page.

Please let me know if you'd prefer a different approach for these error messages!

Finally, what's the workflow for handling the commits that are improvements to previous commits? After the review is complete, should the commits be squashed together?

jeherve · 2019-06-21T15:09:43Z

Yes, it's ready now!

Excellent, I'll take a look when I can.

what's the workflow for handling the commits that are improvements to previous commits? After the review is complete, should the commits be squashed together?

Don't worry about it :) I'll squash all the commits when I merge to master.

jeherve

It looks like Unit Tests are failing right now:

modules/widgets/contact-info/contact-info-admin.js
  line 23  col 9   'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
  line 29  col 13  'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
  line 30  col 13  'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
  line 25  col 17  'ajaxurl' is not defined.
  ⚠  4 warnings

The first 3 warnings are an annoying consequence of us still using jshint (see #11915), but I'm afraid we won't be able to merge as long as the tests are failing.

kbrown9 · 2019-06-21T16:21:52Z

The first 3 warnings are an annoying consequence of us still using jshint (see #11915), but I'm afraid we won't be able to merge as long as the tests are failing.

Thanks for the info! I'll get these fixed up.

kbrown9 · 2019-06-21T17:34:46Z

@jeherve - I amended the commit that caused the jshint test failures. The commit passes the tests now!

kbrown9 · 2019-08-16T02:44:15Z

I still see the too-wide map sometimes:

Thanks - I was able to reproduce the too-wide map with your instructions. I'll look into this.

kbrown9 · 2019-08-16T20:15:28Z

Commit c8544a7 fixes the too-wide map problem by using the admin_enqueue_scripts hook to enqueue the stylesheet on widgets.php.

mdawaffe · 2019-08-16T20:24:46Z

Nice! @kbrown9, could you please rebase this PR on top of the latest master? That will get those failing Travis CI tests to pass. (The tests were fixed in #13245.)

The Google Maps Geocoding API is used to verify that the given address is valid. However, use of this API has recently caused problems for users. Users need to enable two Google Maps APIs in order to use this widget, which isn't user-friendly. Instead of using the Geocoding API results to verify the address, show the embedded map in the widget's admin form. The user can verify that the address is valid and correct by looking at the map. Also, the user will receive Google Maps' error messages if the API key is not valid. Do not show the embedded map In the Customizer admin form. The embedded map isn't necessary in this form because the embedded map is shown in the Customizer preview.

https://codex.wordpress.org/Function_Reference/esc_url_raw

- A few additional comments help understand when has_good_map is needed. - Use Yoda conditions, we must

This avoids that the map breaks out of the widget area layout when displayed.

- This gives more information to users who entered an API key that is not correct for example, instead of just not displaying any map at all.

* Move the logic for determining whether to update the instance's 'goodmap' value from the 'update()' function to a new function, 'update_goodmap()'. * Instead of calling 'has_good_map()' in the 'form()' function, just check the instance's 'goodmap' value. We've already called 'has_good_map()' and saved the result in the instance's 'goodmap' value, so no need to call "has_good_map()" again.

When the Google Maps API returns an error, display it in the widget in the Customizer preview. The CSS properties for the 'contact-map-api-error' class are intended to match the CSS properties for the admin 'notice-warning' class.

Display API key errors in the Contact Info & Map widget's admin form on the Customizer preview page. This requres an Ajax call, which results in a call to the "has_good_map()" method. The "has_good_map()" method checks the API key by calling the Google Maps API. If the API returns an error, that error is displayed in the admin form on the Customizer page.

Addresses Automattic#12474 (review)

Use a nonce to verify that the Ajax request is valid. Also check that the user is allowed to customize before checking the API key from the Ajax request.

Enqueue the contact info stylesheet on widgets.php using the 'admin_enqueue_scripts' hook since widgets.php is an admin page.

* 7.7 changelog: initial set of changes * Changelog: add #13154 * Changelog: add #13134 * Changelog: add #12699 and many others * Changelog: add #13127 * Changelog: add #13167 * Changelog: add #13225 * Changelog: add #13179 * Changelog: add #13173 * Changelog: add #13232 * Changelog: add #13137 * Changelog: add #13172 * Changelog: add #13182 * Changelog: add #13200 * Changelog: add #13244 * Changelog: add #13267 * Changelog: add #13204 * changelog: add #13205 * Changelog: add #13183 * Changelog: add #13278 * Changelog: add #13162 * Changelog: add #13268 * Changelog: add #13286 * Changelog: add #13273 * Changelog: add #12474 * Changelog: add #13085 * Changelog: add #13266 * Changelog: add #13306 * Changelog: add #13311 * Changelog: add #13302 * Changelog: add #13196 * Changelog: add #12733 * Changelog: add #13261 * Changelog: add #13322 * Changelog: add #13333 * Changelog: add #13335

kbrown9 requested a review from a team May 28, 2019 00:54

matticbot added the Touches WP.com Files label May 28, 2019

brbrr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Extra Sidebar Widgets labels May 28, 2019

jeherve added this to the 7.5 milestone May 28, 2019

jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label May 28, 2019

kbrown9 force-pushed the update/contact_info_widget_remove_geocoding branch from 084e01f to 1af852c Compare June 4, 2019 16:53

jeherve mentioned this pull request Jun 5, 2019

Contact Info & Map Widget: improvements to Geocoding changes kbrown9/jetpack#1

Merged

jeherve requested changes Jun 5, 2019

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 5, 2019

kbrown9 force-pushed the update/contact_info_widget_remove_geocoding branch from cdf867d to 7f4b2e0 Compare June 17, 2019 03:54

jeherve removed this from the 7.5 milestone Jun 20, 2019

jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 21, 2019

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 21, 2019

jeherve requested changes Jun 21, 2019

View reviewed changes

kbrown9 force-pushed the update/contact_info_widget_remove_geocoding branch from 15910c9 to f18d932 Compare June 21, 2019 17:12

jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 21, 2019

kbrown9 dismissed jeherve’s stale review via c8544a7 August 16, 2019 20:12

kbrown9 and others added 15 commits August 16, 2019 16:31

Rely on esc_url_raw for remote call

309cbd2

https://codex.wordpress.org/Function_Reference/esc_url_raw

Clarify update process for the map, and coding standard changes

555327a

- A few additional comments help understand when has_good_map is needed. - Use Yoda conditions, we must

Simplify addition of query params to Google embed URL.

6739813

Only attempt to display the map when we have info to display it

e060203

Enqueue CSS on widgets page as well

fe2a033

This avoids that the map breaks out of the widget area layout when displayed.

Display output from API request in widget area when error

f29db56

- This gives more information to users who entered an API key that is not correct for example, instead of just not displaying any map at all.

Contact Info: fix linting warnings in js file

3021b2c

Contact Info CSS: update version so cache can be busted

65e6910

Addresses Automattic#12474 (review)

Do not output notice container when no map has been set up yet

055d08d

Addresses Automattic#12474 (review)

Contact Info & Map Widget: Protect the Ajax request

1f643d6

Use a nonce to verify that the Ajax request is valid. Also check that the user is allowed to customize before checking the API key from the Ajax request.

Fix stylesheet enqueue problem

04cf9ac

Enqueue the contact info stylesheet on widgets.php using the 'admin_enqueue_scripts' hook since widgets.php is an admin page.

kbrown9 force-pushed the update/contact_info_widget_remove_geocoding branch from c8544a7 to 04cf9ac Compare August 16, 2019 20:33

jeherve approved these changes Aug 19, 2019

View reviewed changes

jeherve merged commit e88e82d into Automattic:master Aug 22, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 22, 2019

jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Aug 22, 2019

jeherve added a commit that referenced this pull request Aug 23, 2019

Changelog: add #12474

44809bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contact Info & Map Widget: Remove Geocoding and add map to form #12474

Contact Info & Map Widget: Remove Geocoding and add map to form #12474

kbrown9 commented May 28, 2019 •

edited by jeherve

Loading

jetpackbot commented May 28, 2019 •

edited

Loading

kbrown9 commented May 28, 2019

kbrown9 commented Jun 4, 2019

jeherve left a comment

kbrown9 commented Jun 6, 2019

jeherve commented Jun 20, 2019

kbrown9 commented Jun 20, 2019 •

edited

Loading

jeherve commented Jun 21, 2019

jeherve left a comment

kbrown9 commented Jun 21, 2019 •

edited

Loading

kbrown9 commented Jun 21, 2019

kbrown9 commented Aug 16, 2019

kbrown9 commented Aug 16, 2019

mdawaffe commented Aug 16, 2019 •

edited

Loading

Contact Info & Map Widget: Remove Geocoding and add map to form #12474

Contact Info & Map Widget: Remove Geocoding and add map to form #12474

Conversation

kbrown9 commented May 28, 2019 • edited by jeherve Loading

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

jetpackbot commented May 28, 2019 • edited Loading

kbrown9 commented May 28, 2019

kbrown9 commented Jun 4, 2019

jeherve left a comment

Choose a reason for hiding this comment

kbrown9 commented Jun 6, 2019

jeherve commented Jun 20, 2019

kbrown9 commented Jun 20, 2019 • edited Loading

jeherve commented Jun 21, 2019

jeherve left a comment

Choose a reason for hiding this comment

kbrown9 commented Jun 21, 2019 • edited Loading

kbrown9 commented Jun 21, 2019

kbrown9 commented Aug 16, 2019

kbrown9 commented Aug 16, 2019

mdawaffe commented Aug 16, 2019 • edited Loading

kbrown9 commented May 28, 2019 •

edited by jeherve

Loading

jetpackbot commented May 28, 2019 •

edited

Loading

kbrown9 commented Jun 20, 2019 •

edited

Loading

kbrown9 commented Jun 21, 2019 •

edited

Loading

mdawaffe commented Aug 16, 2019 •

edited

Loading