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: Persist Center Point #14417

Merged
merged 2 commits into from
Jan 22, 2020
Merged

Conversation

jeffersonrabb
Copy link
Contributor

Changes proposed in this Pull Request:

This PR makes a few changes to the calculation and persistence of the Map block center point. The block determines the center point based on the map points that have been set. When the block initializes, the map "travels" to correct position and zoom, which causes a dramatic and unnecessary animation. This PR persists the true center point of the map, allowing the map to initialize in that position, avoiding the animation.

This PR also adds a bonus feature: in a map with no points defined, the editor can determine the center point by dragging, and this position will be respected when the map renders.

If any points have been chosen, dragging is disabled. This is done to avoid the false expectation that the new position will be respected, since the block will replace it with the calculated center point based on the available points.

Based on Automattic/wp-calypso#28811.

Testing instructions:

  • On master, add three Map blocks to a page with 0 points, 1 point, and 2 points.
  • Switch to fix/map-block-center-point, verify that the blocks remain valid and work as expected.
  • Create a new post and add Map blocks to it: 1) no points defined, left in original position (SF, CA) 2) no points defined, dragged somewhere else 3) one point defined 4) multiple points defined
  • Verify the positions of all four remain intact after saving and reloading the editor, and that there is no initial animation.
  • Verify the position of all four are identical in the front end, and that there is no initial animation.
  • Verify the first two can be dragged, and the second two cannot.

…en map loads. allows centering the map without adding any pins.
@matticbot
Copy link
Contributor

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

@jeffersonrabb jeffersonrabb requested a review from Copons January 21, 2020 13:55
@jeffersonrabb jeffersonrabb self-assigned this Jan 21, 2020
@jeffersonrabb jeffersonrabb added [Block] Map [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 21, 2020
@jetpackbot
Copy link

jetpackbot commented Jan 21, 2020

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 736ea75

Copy link
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.

This works great and is an awesome improvement over the previous behaviour!
I've left a couple of minor comments, but the PR is pretty much good to go! ✨

extensions/blocks/map/component.js Show resolved Hide resolved
extensions/blocks/map/component.js Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

jeffersonrabb, Your synced wpcom patch D37943-code has been updated.

@jeherve jeherve added this to the 8.2 milestone Jan 22, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me! 🚢

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 22, 2020
@jeffersonrabb jeffersonrabb merged commit 25f7032 into master Jan 22, 2020
@jeffersonrabb jeffersonrabb deleted the fix/map-block-center-point branch January 22, 2020 16:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 22, 2020
@jeherve
Copy link
Member

jeherve commented Jan 23, 2020

r202023-wpcom

jeherve added a commit that referenced this pull request Jan 27, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
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.

5 participants