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

Simple Payments: pre-populate attributes when converting from classic block #11731

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Mar 27, 2019

Changes proposed in this Pull Request:

It populates rightly the product properties gotten from the data/response when it comes from a shortcode transformation.

Fixes Automattic/wp-calypso#29870

Testing instructions:

  1. Get a post with a "Simple Payment Button". If it doesn't exist just create it:
    a. Get the product ID (inspect the dom tree trying to find the ID, or inspect the XHR request from the network tab)
    b. Once you get the product ID, create a new Classic block with the Simple Payments shortcode:
[simple-payment id="<product-id"]

image

  1. Convert to Blocks
    image

  2. Once it's converted it should populate the fields with the right data.
    populate-simple-payments

Proposed changelog entry for your changes:

Simple Payments Button: Convert classic Blocks (shortcode) rightly.

@retrofox retrofox added [Status] In Progress Simple Payments [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 27, 2019
@retrofox retrofox requested a review from a team March 27, 2019 20:40
@gwwar gwwar requested a review from a team March 27, 2019 20:48
@ghost ghost assigned retrofox Mar 27, 2019
@ghost ghost added the in progress label Mar 27, 2019
@retrofox retrofox added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed in progress labels Mar 27, 2019
@retrofox retrofox changed the title [wip] Simple Payments Button: populates rightly the product Simple Payments Button: populates rightly the product Mar 27, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 28, 2019

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 29, 2019.
Scheduled code freeze: April 22, 2019

Generated by 🚫 dangerJS against 341f90c

mmtr
mmtr previously approved these changes Mar 28, 2019
Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Nice fix! It worked as expected when I converted to blocks a existing post created in the classic editor that was containing a payment button.

extensions/blocks/simple-payments/edit.js Outdated Show resolved Hide resolved
@jeherve jeherve requested a review from a team March 28, 2019 11:15
@retrofox retrofox force-pushed the fix/simple-payment-convert-to-block branch from b330bec to b3025c7 Compare March 28, 2019 12:34
@simison simison added this to the 7.2 milestone Mar 28, 2019
@simison simison added the [Type] Bug When a feature is broken and / or not performing as intended label Mar 28, 2019
@retrofox retrofox changed the title Simple Payments Button: populates rightly the product Simple Payments: pre-populate attributes when converting from classic block Mar 28, 2019
@simison
Copy link
Member

simison commented Mar 28, 2019

@kraftbj FYI I've added this to 7.2 milestone as it's a bugfix, in case it makes it in before we ship blocks. Lemme know if you'd prefer to punt it for 7.3

@simison

This comment has been minimized.

@simison

This comment has been minimized.

@kraftbj
Copy link
Contributor

kraftbj commented Mar 28, 2019

Since it isn't a regression caused by 7.2, I would prefer to punt to 7.3 if it isn't critical. We've been burnt before by sliding in fixes late that caused new issues (I've done plenty of that burning).

One advantage of waiting too is don't need to worry about cherry-picking cross-repo :)

@simison
Copy link
Member

simison commented Mar 28, 2019 via email

mmtr
mmtr previously approved these changes Mar 28, 2019
rodrigoi
rodrigoi previously approved these changes Mar 29, 2019
Copy link
Contributor

@rodrigoi rodrigoi left a comment

Choose a reason for hiding this comment

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

🚀 🌕 !

@rodrigoi rodrigoi 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 Mar 29, 2019
@rodrigoi rodrigoi modified the milestones: 7.2, 7.3 Mar 29, 2019
@roccotripaldi
Copy link
Member

I think punting to 7.3 should be fine, but I'd like to get this on master ASAP so that we can fix the problem for wpcom sites.

jeherve
jeherve previously approved these changes Apr 2, 2019
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 works well in my tests. 👍

retrofox added 3 commits April 2, 2019 16:29
it populates rightly the product properties gotten from the data/response when it comes from a shortcode transformation.
@roccotripaldi roccotripaldi dismissed stale reviews from jeherve, rodrigoi, and mmtr via 341f90c April 2, 2019 20:29
@roccotripaldi roccotripaldi force-pushed the fix/simple-payment-convert-to-block branch from b3025c7 to 341f90c Compare April 2, 2019 20:29
@matticbot
Copy link
Contributor

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

@obenland
Copy link
Member

obenland commented Apr 2, 2019

@roccotripaldi Damian is on leave, would you be available to bring this over the finish line?
/cc @vindl

@roccotripaldi
Copy link
Member

would you be available to bring this over the finish line?

@obenland
Yeah, no problem. I'll see that it's merged here, and on wpcom!

@obenland obenland assigned roccotripaldi and unassigned retrofox Apr 2, 2019
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Works as advertised.

@kraftbj kraftbj merged commit 029e915 into master Apr 2, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 2, 2019
@kraftbj kraftbj deleted the fix/simple-payment-convert-to-block branch April 2, 2019 21:53
kraftbj added a commit that referenced this pull request Apr 30, 2019
@jeherve jeherve added the [Feature] Pay with PayPal aka Simple Payments label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Pay with PayPal aka Simple Payments [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg: Simple Payments in Classic Block converts to an empty Simple Payments block
10 participants