Map Block

[jeffersonrabb]

Changes proposed in this Pull Request

• Add Map block to repo, for inclusion in Jetpack.

Preview

Screen capture: https://cloudup.com/ceQ6jKr5-R4
Atavist block example (this block is an adaptation of the Atavist version): https://wpmigration.atavist.com/maps-on-atavist

Testing instructions

Jurassic Ninja

• Click this link: https://jurassic.ninja/create/?gutenberg&gutenpack&shortlived&jetpack-beta&calypsobranch=atavist/map-block-v2&branch=try/google-maps-api-key
• Set Up Jetpack/Free Tier
• Activate Recommended Features

Jetpack/Docker

• Setup and run the Jetpack docker environment.
• In Jetpack repo, checkout try/google-maps-api-key branch.
• In wp-calypso repo, checkout atavist/map-block-v2 branch.
• Run the following from the root of wp-calypso repo`: npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- --output-dir=PATH_TO_JETPACK_REPO/jetpack/_inc/blocks ( replace PATH_TO_JETPACK_REPO with the path to your local Jetpack repo).

Vagrant (No longer valid following changes to Google Maps API Key handling)

Some questions for discussion

1. The block uses browser Geolocation, if available. If no map markers are defined, the block will attempt to use the user's current location. This means that browser will ask for permission to access location, and it significantly slows the initial load. Is this a worthwhile feature? Or does the permission request seem invasive and the delay annoying? If we do opt to keep the feature, at minimum we'll need a loading screen.
~~2. How should we handle the Google Maps API key? Currently an API key is hardcoded in the config file, but this is not a sustainable solution. Google requires that a valid key be used in all requests, which must be tied to a billing account. ~~
3. The library used to dynamically load the Google Maps script is loadScript. Because of this issue, importing the library causes errors so as a temporary fix I duplicated the library and placed it in the block directory here. After the issue is resolved, I should be able to remove the duplicate and import the original library.
5. The sidebar includes the Locations component which allows editing of title/caption for all defined markers. The same editing is possible by clicking on markers on the map, which is the more "Gutenberg-esque" approach. I plan to remove the Locations component, unless people feel that both are necessary.
6. The Map Theme Picker uses images of the themes. SDK builds fail if an image is included (issue here) so I am temporarily accessing those images from a public S3 bucket.
7. Any additional map styles we should include?
8. Should a future iteration of the block support other mapping services?

[matticbot]

Test live: https://calypso.live/?branch=atavist/map-block-v2

[MichaelArestad]

This is awesome!

As we discussed earlier, there are some questions about the caption:

1. Atavist has some custom styling for captions. Do we need the caption styles to match the Atavist Map caption?
2. If not, do we need a caption at all?

On other Gutenberg blocks, we have been allowing folks to add their own blocks to label or caption elements. For example, the subscription form will just be an email input and submit button. We are relying on folks to add a paragraph or heading encouraging folks to use the form. I think we could do something similar here by removing the caption which would slightly simplify this block.

However, if we need it for parity, maybe we could have an "Atavist site" mode and a default mode. In the Atavist mode, we show the caption/controls. In the default mode we don't. I am wary of this because it might be easy to forget to support down the road.

There is also a minor padding issue on the marker where the address collides with the arrow. Easy peasy to fix with a little padding:

I also noticed the input for the address (with autocomplete) is contenteditable. I'm not sure what the best move here is as it was easy enough to paste in web content into the field. I'll leave judgement to Luna on this.

I also think we could tweak the color palette of the pins. I'll see what I can do there. I think our brand team has some color palettes we could probably pull from.

[jeffersonrabb]

@MichaelArestad, a few notes on your comments:

I also think we could tweak the color palette of the pins.

I changed the color palettes to use Theme colors, matching the Paragraph block background and others. 4af0541

There is also a minor padding issue on the marker where the address collides with the arrow.

Fixed in 6776046

On 10/12 @MichaelArestad found out a few issues that caused map markers to stop responding to clicks. These included multiple map blocks in one page, removing and re-adding map block without page refresh, opening a marker and closing the info window by clicking the X. These are now resolved in 4d91781

do we need a caption at all

It looks like all of the media blocks (image|video|audio) have captions, but no others do. Atavist Maps do have a caption, which is styled differently, so in the interest of aiming for full visual parity I'll need some way to do this. Here are the options I can think of:

1. Create "official" Map Block without caption, use Gutenberg hook & filter extensibility options to append a caption for Atavist migration customers.
2. Include caption in the Map Block, but have it hidden by default. Add sidebar toggle to switch it on/off.
3. Remove caption from Map block, add caption content as styled paragraph blocks in Atavist migration.
4. Consider Map to be more like Image/Video/Audio, include caption as is current state.

What do you think?

[jeffersonrabb]

I also noticed the input for the address (with autocomplete) is contenteditable. I'm not sure what the best move here is as it was easy enough to paste in web content into the field.

Gutenberg Autocomplete is really designed to work with a contenteditable element, not a plain text input. In the first version of this PR I had a hacky solution working, but as @MichaelArestad points out its kind of gross to have a contenteditable element masquerading as a text input. I ended up writing a new component, Lookup, which is based on Autocomplete as well as @ryelle's work here. Lookup differs from Autocomplete in the following ways:

• Designed to be used with a text input instead of a contenteditable div.
• Takes only one completer param (for a text input there is only need for one, while a Rich Text element can use many)
• No TriggerPrefixes at all, since anything typed in the input should be immediately checked for a match.
• No node-level assessment and disabling, since the input is a simple text field.

As of 042b55b Lookup is functional, although I still need to add key command and spoken message support to match Autocomplete. At a later stage I might propose including it as a core Gutenberg component since I expect this is a use case which will come up again. cc: @mtias, @ryelle

[MichaelArestad]

Create "official" Map Block without caption, use Gutenberg hook & filter extensibility options to append a caption for Atavist migration customers

This seems like the cleanest approach. I wonder if the caption could even be a separate block that's hidden for most users?

[jeffersonrabb]

Caption removed in 482b271. Note that if you have a pre-existing Map block you'll get errors and need to remove and replace. Since the block hasn't been released yet, seems like overkill to set up deprecation for the caption.

[tyxla]

This is amazing work! I've left some comments, and questions, but I must admit this looks really impressive!

[tyxla]

Ouch, a PHP plugin in Calypso. Doesn't seem to be the right place for this - if this was going in Jetpack, I'd suggest an accompanying PR in Jetpack instead. Then we wouldn't be needing any of it if we use the Jetpack blocks preset.

[simison]

This feels still an open question.

If the Atavist block should always depend on Jetpack, then it makes sense to move PHP to Jetpack as well. If it needs to work with both Jetpack and Atavist somewhere else without Jetpack, it gets more complicated.

cc @dmsnell

[jeffersonrabb]

Discussion here: p1537557809000100-slack-jetpack-gutenberg

[dmsnell]

yeah 🤷‍♂️ we're playing around with it. there's no reason PHP can't live in Calypso's repo. I think it's worthwhile exploring but I don't have an answer.

[sirreal]

This will require Automattic/jetpack#10362 or similar for locales to work properly on the frontend.

[enejb]

Is this code necessary? I though that Jetpack would determine when this code gets loaded.

[jeffersonrabb]

This file is actually unrelated to Jetpack. The Map block is going to be included in Jetpack but this (and other similar blocks) are also being included in a stand-alone plugin, and this .php file relates to the plugin only.

[enejb]

I did some testing and it works really well. Nicely done.

I noticed that the code creates the following block html.

<!-- wp:atavist/maps {"zoom":9} -->
<div class="wp-block-atavist-maps atavist-block atavist-simple-map atavist-block-align-center" data-map_style="default" data-points="[]" data-zoom="9" data-map_center="{&quot;latitude&quot;:37.7749295,&quot;longitude&quot;:-122.41941550000001}" data-marker_color="red" data-api_key="code...."></div>
<!-- /wp:atavist/maps -->

I think if the code is being included in Jetpack we should prefix it with Jetpack instead of atavist. This way the user would know that it they would need to reactivate Jetpack to get the component to work again. I am also wonder if we could use some sort google maps iframe or image tag as the default view instead of a div. This way the map block would should something to the user even in environments where js is not supported. Such as when the content appears inside a feed reader, a subscription email or when the plugin is deactivated.

When it comes to the google maps api key on the .com side we have a few different filters that you can use that will return the expected api key. We currently do not expose thats key in Jetpack even for the paid users.

They are required to enter it as part of the widget UI.
See:

What the plan is for the other stand alone plugin? Where can I read more about it?

[oskosk]

Seems like Mapbox refers to the secret as Access tokens instead of keys. I found it confusing because I just read the placeholder text mildly. Although I copied what seemd to be my Default Public Token and the map just worked.

Should we take this in consideration and update the terms to be consistent with each other ? It's properly referred to as "Access token" on the inspector controls (sidebar), above the text input for the token, but the buttons to remove or edit still refer to it as key.

[jeffersonrabb]

Seems like Mapbox refers to the secret as Access tokens instead of keys.

Holding on this until we determine which service we're using, but if we land on Mapbox I'll make sure this text change happens.

[simison]

Perhaps this PR could be merged and iterated further in smaller follow up PRs?

It can be left out of the preset-files before we publish to NPM in case you don't want to include in Jetpack plugin yet.

Just needs a rebase first and two tests are failing on Circle CI.

[roccotripaldi]

Let's get this removed from the preset and rebased.

[jeherve]

I assume this can now be closed, since the block was merged in #28498?

[jeffersonrabb]

I assume this can now be closed, since the block was merged in #28498?

Correct. I'm closing now.