Skip to content

Map Block: Use the block styles API for choosing map themes#14852

Merged
pento merged 14 commits intomasterfrom
fix/14559-block-style
Mar 26, 2020
Merged

Map Block: Use the block styles API for choosing map themes#14852
pento merged 14 commits intomasterfrom
fix/14559-block-style

Conversation

@pento
Copy link
Copy Markdown
Contributor

@pento pento commented Mar 2, 2020

This PR switches the map block over to using the block styles attribute, rather than the custom theme picker.

Fixes #14559

Changes proposed in this Pull Request:

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

  • Change to existing feature.

Testing instructions:

  • Add a new map block, test that the block style switches the map theme as expected.
  • Open an post with an old map block, check that in correctly converts to the new style.

Proposed changelog entry for your changes:

  • Map Block: Improve the map theme picker.

@pento pento added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Status] Needs Testing We need to add this change to the testing call for this month's release labels Mar 2, 2020
@pento pento requested review from a team March 2, 2020 07:44
@pento pento self-assigned this Mar 2, 2020
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 2, 2020 •
edited
Loading

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: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 07190e9

@pento
Copy link
Copy Markdown
Contributor Author

pento commented Mar 2, 2020

Some early notes:

  • The Styles panel defaults to being closed, there's no way to override that. This is a change in behaviour from the existing theme picker.
  • I couldn't figure out the purpose of the "Default Style" dropdown which appears in the Style panel. Also not sure how to disable it.
  • I haven't tested frontend rendering, either of old or new map blocks. They're almost certainly broken at the moment. 🙂

@scruffian
Copy link
Copy Markdown
Member

scruffian commented Mar 2, 2020

The Styles panel defaults to being closed, there's no way to override that. This is a change in behaviour from the existing theme picker.

Sounds like we should open a core patch for that

@scruffian
Copy link
Copy Markdown
Member

scruffian commented Mar 2, 2020 •
edited
Loading

I haven't tested frontend rendering, either of old or new map blocks. They're almost certainly broken at the moment. 🙂

It did work before this change, but now it doesn't :)

scruffian
scruffian previously requested changes Mar 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

This is a really good direction. We have a custom component which does something similar, but if we can do it this way that would be ideal!

We just need to get it working on the frontend...

@scruffian
Copy link
Copy Markdown
Member

scruffian commented Mar 2, 2020

There's an annoying margin at the top of the previews, which I have a fix for here: WordPress/gutenberg#19983

pablinos
pablinos reviewed Mar 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@pablinos pablinos left a comment

Choose a reason for hiding this comment

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

I agree with @scruffian, this is a better pattern than we have at the moment. Let's get it working along with any core patches needed!

@apeatling
Copy link
Copy Markdown
Contributor

apeatling commented Mar 3, 2020

Ran through and tested this and it worked as expected and mentioned in the instructions. I like the experience of using block styles for this. Agreed that we should submit a core PR to allow the style panel to be open by default, it'll be incredibly easy to miss this otherwise.

This was referenced Mar 4, 2020
Merged
Merged
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 4, 2020

Caution: This PR has changes that must be merged to WordPress.com
Hello pento! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39810-code before merging this PR. Thank you!

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 4, 2020

pento, Your synced wpcom patch D39810-code has been updated.

1 similar comment
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 4, 2020

pento, Your synced wpcom patch D39810-code has been updated.

@pento pento requested a review from scruffian March 4, 2020 08:23
@pento
Copy link
Copy Markdown
Contributor Author

pento commented Mar 4, 2020

This PR has generated two Gutenberg PRs:

I've also switched back to storing the map style in the data-map-style attribute (e2927a6), which has the added bonus of no longer needing a deprecated block definition, and we're not changing the save() output, so we don't have to hold merging this to WPCOM until the end of the month. 🙂

@pento pento added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 4, 2020
scruffian
scruffian reviewed Mar 4, 2020
View reviewed changes
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 4, 2020

pento, Your synced wpcom patch D39810-code has been updated.

@scruffian
Copy link
Copy Markdown
Member

scruffian commented Mar 4, 2020

I added a commit to bring this into line with how we do things with Tiled Gallery. It also makes the code a bit simpler.

Copons
Copons previously approved these changes Mar 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

LGTM!

@pento pento force-pushed the fix/14559-block-style branch from 31537fa to 285323a Compare March 26, 2020 02:09
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 26, 2020

pento, Your synced wpcom patch D39810-code has been updated.

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 26, 2020

pento, Your synced wpcom patch D39810-code has been updated.

@pento pento requested a review from jeherve March 26, 2020 03:11
@pento pento added [Status] Needs Review This PR is ready for review. 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 Mar 26, 2020
Copons
Copons approved these changes Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

Hopefully for the last time 😄

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 26, 2020

pento, Your synced wpcom patch D39810-code has been updated.

kraftbj
kraftbj approved these changes Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tested it on a fresh install and on an install bringing in an older version of the block. Both worked as expected as described in the ticket.

Fusion is failing because of new files. I'm working on that now to update the phab.

@jeherve jeherve 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 Mar 26, 2020
@pento pento dismissed jeherve’s stale review March 26, 2020 21:48

Issues addressed.

@pento
Copy link
Copy Markdown
Contributor Author

pento commented Mar 26, 2020

@Automattic/jetpack-crew: Could someone merge this, please? It looks like the tests are now passing on the diff (thanks, @kraftbj!), but it hasn't synced the status, so merge is blocked for me.

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Mar 26, 2020

@pento Unblocked! Please merge this (and the corresponding WP.com diff) at your leisure.

@pento pento merged commit d7fb67e into master Mar 26, 2020
@pento pento deleted the fix/14559-block-style branch March 26, 2020 22:36
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
Chanegelog: add #14852
4ecb091
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scruffian scruffian scruffian left review comments

@kraftbj kraftbj kraftbj approved these changes

@Copons Copons Copons approved these changes

@jeherve jeherve Awaiting requested review from jeherve

+1 more reviewer

@pablinos pablinos pablinos left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pento pento

Labels

[Block] Map Enhancement Changes to an existing feature — removing, adding, or changing parts of it Touches WP.com Files

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

Map Block: UI for map themes

10 participants

@pento @jetpackbot @scruffian @apeatling @matticbot @kraftbj @pablinos @jeherve @Copons @creativecoder