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

Feat/donate block tiers layout #1311

Merged
merged 31 commits into from
Dec 15, 2022
Merged

Conversation

adekbadek
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Adds an alternative, "tiers-based" layout to the Donate block:

image

Above in the "Alternate" block style version

It's called "Tiers-based" layout (vs. the default "Frequency-based" layout), but I'm not sure about the naming here. Suggestions are welcome.

image

Closes https://github.com/Automattic/newspack-product/issues/53

How to test the changes in this Pull Request:

  1. Set Stripe as the Reader Revenue platform
  2. On master, create a page with three Donate blocks, set each to different style
  3. Switch to this branch, ensure these all render as expected
  4. Switch all to "Tiers-first" in the sidebar "Layout" panel
  5. Visit the front-end, make a few donations using the new block layout, observe amounts and frequencies translating to Stripe

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo
Copy link
Contributor

dkoo commented Nov 17, 2022

Haven't reviewed yet, but for this:

It's called "Tiers-based" layout (vs. the default "Frequency-based" layout), but I'm not sure about the naming here. Suggestions are welcome.

I'd suggest just "Frequency" and "Tiers".

@thomasguillot
Copy link
Contributor

This is cool!

I'm very pleased with this. Just a few questions/suggestions:

  • Can we style the 3 buttons differently? Ideally the "best value" box should have a filled button but the other 2 boxes should have outlined buttons. So we have 1 primary CTA and 2 secondary CTAs.
  • Can we make the "best value" label background/text colours customisable?
  • Same with the "best value" black border, could we customise this? (thickness + colour inc. gradient)

@adekbadek
Copy link
Member Author

Can we style the 3 buttons differently? Ideally the "best value" box should have a filled button but the other 2 boxes should have outlined buttons. So we have 1 primary CTA and 2 secondary CTAs.

Done in 49a99ef :

image

Can we make the "best value" label background/text colours customisable?
Same with the "best value" black border, could we customise this? (thickness + colour inc. gradient)

Please submit a separate issue for this, this PR is pretty large as it is.

@dkoo dkoo force-pushed the feat/donate-block-tiers-layout branch from 1260b51 to d7bca81 Compare December 5, 2022 20:13
Copy link
Contributor

@dkoo dkoo 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 big new feature—thanks for implementing! It's mostly clean, and I confirmed that existing Donate blocks continue to behave nicely with the new features as well.

I did run into some issues, some questions, and some non-blocking nitpicks.

src/blocks/donate/edit/TierBasedLayout.tsx Show resolved Hide resolved
src/blocks/donate/tiers-based/style.scss Show resolved Hide resolved
src/blocks/donate/block.json Outdated Show resolved Hide resolved
src/blocks/donate/consts.ts Outdated Show resolved Hide resolved
src/blocks/donate/edit/index.tsx Outdated Show resolved Hide resolved
src/blocks/donate/edit/index.tsx Show resolved Hide resolved
src/blocks/donate/tiers-based/slider.ts Show resolved Hide resolved
src/blocks/donate/tiers-based/view.ts Show resolved Hide resolved
@adekbadek adekbadek requested a review from dkoo December 9, 2022 14:19
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Seeing some new behavior now which needs mitigation.

  1. For some reason, on the front-end the block form loads without a selected state. Confirmed that this happens for both new and pre-existing block instances, and both manually-configured and default settings. The block looks like this on initial page load, and doesn't show any tier info until I click on the Monthly or Annual tabs:

Screen Shot 2022-12-09 at 2 46 11 PM

  1. When I submit the form, the JS doesn't seem to capture the submit event. On AMP or AMP Plus, the page simply reloads with my form inputs appended to the URL (as in default HTML form submit behavior), and on non-AMP, submitting the form does nothing.

  2. If the default Reader Revenue > Donation settings are untiered, then "Tiers" still appears as a layout option for Donate blocks, but is unselectable.

All issues confirmed resolved after the latest round of changes.

Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

@adekbadek thank you for all of the updates and improvements on this PR. Getting really close now. There are still a couple of minor unresolved threads above, and I found another minor bug which I hadn't noticed before.

On the Stripe-enabled block, if you submit a tiered form by filling out all required fields and hitting "enter" (on the keyboard), the form display goes back to the initial view with all tiers, and the success message isn't visible.

@adekbadek
Copy link
Member Author

For some reason, on the front-end the block form loads without a selected state.

Fixed in de2ae2b!

When I submit the form, the JS doesn't seem to capture the submit event.

Can't reproduce that. As concluded in a DM, this is not reproducible, must've been a build issue.

If the default Reader Revenue > Donation settings are untiered, then "Tiers" still appears as a layout option for Donate blocks, but is unselectable.

Fixed in de6212e

@adekbadek adekbadek requested a review from dkoo December 13, 2022 11:15
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Preapproving since the last remaining thread may not be an issue at all. Feel free to merge after @thomasguillot weighs in on the desired behavior.

@adekbadek adekbadek merged commit 9228ee4 into master Dec 15, 2022
@adekbadek adekbadek deleted the feat/donate-block-tiers-layout branch December 15, 2022 12:11
matticbot pushed a commit that referenced this pull request Dec 20, 2022
# [1.60.0-alpha.1](v1.59.1...v1.60.0-alpha.1) (2022-12-20)

### Bug Fixes

* allow all img tag attributes in avatar output ([#1333](#1333)) ([2f57019](2f57019))

### Features

* **donate block:** tiers layout ([#1311](#1311)) ([9228ee4](9228ee4))
* handle 'other' reader revenue platform in the editor ([0e6ca8a](0e6ca8a))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.60.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jan 9, 2023
# [1.60.0](v1.59.1...v1.60.0) (2023-01-09)

### Bug Fixes

* allow all img tag attributes in avatar output ([#1333](#1333)) ([2f57019](2f57019))

### Features

* **donate block:** tiers layout ([#1311](#1311)) ([9228ee4](9228ee4))
* **donate:** additional fields on the form ([#1330](#1330)) ([1f6869d](1f6869d))
* handle 'other' reader revenue platform in the editor ([0e6ca8a](0e6ca8a))
* unregister Jetpack Subscriptions block to avoid confusion with Newspack blocks ([#1337](#1337)) ([7dab5fc](7dab5fc))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.60.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants