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

Map block: Add address block attribute #21412

Merged
merged 13 commits into from
Oct 25, 2021
Merged

Conversation

ivan-ottinger
Copy link
Contributor

@ivan-ottinger ivan-ottinger commented Oct 14, 2021

The Map block component currently doesn't provide a way to create a map point from a physical address, e.g. "Hrad Devín, Slovanské nabrežie, Bratislava, Bratislava 841 10, Slovakia".

In other words, when we want to create a Map block in our code, we need to provide geographical coordinates, for example:

createBlock( 'jetpack/map', {
		points: [ {
			coordinates: {
				latitude: 48.14472,
				longitude: 17.11278
			}
		} ],
	} );

The changes proposed in this PR add a new address block attribute that can be used in the code as follows (example):

createBlock( 'jetpack/map', { address: 'Hrad Devín, Slovanské nabrežie, Bratislava, Bratislava 841 10, Slovakia', } );

When the address attribute is present, the Map block will create a map pointing to the location at the specified address. We don't need to know the location coordinates.

This PR was created as a part of solution for #21232: Contact Info & Map widget: Add legacy widget → block transform.

Changes proposed in this Pull Request:

  • Add new getCoordinates() function that fetches location (point) data using Mapbox API
  • Add new geoCodeAddress() function that translates fetched data into a map point
  • Change apiCall() to return a promise to make sure geoCodeAddress() is run only once we have the Mapbox API key ready

Does this pull request change what data or activity we track or use?

No, it does not.

Testing instructions:

  • The proposed changes are utilized in the following PR where we can test them: Contact Info & Map widget: Add legacy widget → block transform #21232 (please see the "Testing instructions:" section)
  • There should be no other change to the functionality of the Map block. For example, adding points to a Map block inserted to a page or a post should work as expected.

Special thanks goes to @alshakero and @yansern for their valuable insights and guidance.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ivan-ottinger! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D68417-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added [Block] Map [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Oct 14, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: November 2, 2021.
  • Scheduled code freeze: October 25, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Oct 14, 2021
@ivan-ottinger ivan-ottinger force-pushed the add/map-block-address-attribute branch from ee19bb5 to 53925ff Compare October 14, 2021 14:58
@ivan-ottinger ivan-ottinger marked this pull request as ready for review October 15, 2021 10:37
Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Some minor but important requests.

Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Awesome work. Left a few cosmetic comments, one potential bug.

Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Adding to the above

alshakero
alshakero previously approved these changes Oct 22, 2021
Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

I couldn't find a straight-forward way to get the user's language in Jetpack. This can be merged now. LGTM!

Map block component currently  doesn't provide a way to create a map from provided address.

This commit adds `address` block attribute that can then be geocoded  and used to render the map (as opposed to only using coordinates (lat, long)).
The `getCoordinates` function will take care of the Mapbox API request.

The function accepts physical address and Mapbox API key as parameters.

The output is the promise / Mapbox API response that contains geo coordinates.
`geoCodeAddress` will take care of creating the new map point based on the geo coordinates.

This commit also makes sure that the above function is run when the Map component updates (`componentDidUpdate`).

The function runs after the Mapbox API key is ready.
This change will make sure there's  specific error message displayed in the front-end - as opposed to only displaying error in the console.
This commit handles situations where the address provided by the user cannot be properly localized.

If the Mapbox API returns empty array of locations (`result.features`), we will display an error message.
The commit introduces a new const
`feature` that is then used as a building block for `newPoint` object values.
@ivan-ottinger ivan-ottinger force-pushed the add/map-block-address-attribute branch from 3ee6839 to 65fb66e Compare October 22, 2021 11:18
@ivan-ottinger ivan-ottinger removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Oct 22, 2021
alshakero
alshakero previously approved these changes Oct 22, 2021
Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Approving for the bots

@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 22, 2021
@samiff samiff self-requested a review October 22, 2021 22:57
samiff
samiff previously approved these changes Oct 22, 2021
Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

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

Tested well for me 👍 Tried to break the address during transform, but was able to see the helpful error notice as expected.

It looks like all feedback was addressed in #21232 but just wanted to double check with you @ivan-ottinger ? If so go ahead and merge #21232 and then we'll just need to make sure we have a complete wpcom diff ready to go.

@samiff samiff added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 22, 2021
@ivan-ottinger
Copy link
Contributor Author

It looks like all feedback was addressed in #21232 but just wanted to double check with you @ivan-ottinger ?

Thank you @samiff, yes most of the feedback was addressed there.

There's also an open discussion in regards the mapping service: p7fD6U-4KX-p2, but it doesn't seem to me to be a blocker. Pinging @jeherve just in case you would like to share something. :)

If so go ahead and merge #21232 and then we'll just need to make sure we have a complete wpcom diff ready to go.

Just to make sure everything is clear, the change in #21232 requires the change proposed in this PR (#21412) to work properly.

In other words, the change in #21232 already expects the address attribute (introduced in this PR #21412) to be available when creating new Map block:

createBlock( 'jetpack/address', {
	address: instance.raw.address,
} ),

If I understand correctly, we need both PRs to be merged and then we will need to create a complete WPCOM diff and then deploy it at the proper time, right?

* Remove Contact Info & Map widget from legacy-widget block

* Add changelog file

* Add transform of the legacy widget to  relevant block

This commit adds transformation of the "Contact Info & Maps" widget into "Contact Info" block, plus extra necessary blocks.

* Update changelog file

* Code refactor - construcing `innerBlocks` directly

This commit removes unnecessary variables and builds `innerBlocks` array directly.

* Add Map block to the transform

If the `Show map` checkbox is selected, Map block is added during the transform.

* Add conditional to the `Hours` field

Make sure that the extra Paragraph block is added only when the `Hours` field has some content.

* Remove `address` attribute from the Map block

This commit reverts the changes made in the previous commit as it makes more sense to introduce Map block changes in a separate branch/PR.

* Use the new Map block `address` field

* Add `address` attribute to the Map block

This commit adds the `address` attribute back after I had incorrectly removed it.
@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 25, 2021
@samiff samiff enabled auto-merge (squash) October 25, 2021 20:40
@samiff
Copy link
Contributor

samiff commented Oct 25, 2021

Just to make sure everything is clear, the change in #21232 requires the change proposed in this PR (#21412) to work properly.

@ivan-ottinger Sorry if that wasn't clear, since #21232 was branched off from this PR, I just meant that it'd need to be merged first, then we could make sure tests were still passing here and that there was a wpcom diff with both PR changes. Looks like the wpcom diff automagically happens, so D67571-code is ready to go 👍

@samiff samiff merged commit 2f352c6 into master Oct 25, 2021
@samiff samiff deleted the add/map-block-address-attribute branch October 25, 2021 20:41
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D68417-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 25, 2021
davidlonjon added a commit that referenced this pull request Oct 26, 2021
* master: (27 commits)
  Admin Page: Update upgrade buttons Tracks events (#21483)
  Debug Helper IDC Simulator: Add the ability to spoof the home url (#21516)
  Map block: Add `address` block attribute (#21412)
  Update wordpress monorepo (#21522)
  Stats Page: Add VideoPress nudge (#21513)
  Improve Jetpack products illustrations file size (#21514)
  Update dependency mediawiki/mediawiki-codesniffer to v38 (#21523)
  Update dependency yoast/phpunit-polyfills to v1.0.2 (#21524)
  Widgets: add Strava to Social Icons Widget and Social Menu (#21518)
  Boost: Add active modules to speed score request (#21471)
  Markdown: Better accessibility for Footnotes (#21495)
  Jetpack Plugin: Add VideoPress upgrade page (#21497)
  Admin Menu: moves Add new (plugin) menu item to the top for Atomic sites (#21506)
  General: Check the return value of get_comment() (#21080)
  Instagram Widget: link to user connection if user is not connected (#21512)
  Search: Create a new search package, rename Search plugin (#21502)
  Identity Crisis: Remove the unused jetpack_idc_option transient (#21492)
  Stats: do not update the wpcom blog details (#21441)
  Admin: do not display upgrade messages when on offline mode (#21444)
  jetpack: Catch calls to `exit()` and `die()` in PHPUnit (#21043)
  ...
@ivan-ottinger
Copy link
Contributor Author

This is great!

Sorry if that wasn't clear, since #21232 was branched off from this PR, I just meant that it'd need to be merged first, then we could make sure tests were still passing here and that there was a wpcom diff with both PR changes. Looks like the wpcom diff automagically happens, so D67571-code is ready to go 👍

Thank you for clarifying Sami! That helped! :)

I will deploy the patch to WPCOM and then add a new comment with changeset here.

@ivan-ottinger
Copy link
Contributor Author

Successfully deployed to WPCOM. Changeset: r233943-wpcom.

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

Successfully merging this pull request may close these issues.

5 participants