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

fix: donate block layout for NRH #2954

Merged
merged 1 commit into from
Feb 29, 2024
Merged

fix: donate block layout for NRH #2954

merged 1 commit into from
Feb 29, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Feb 28, 2024

All Submissions:

Changes proposed in this Pull Request:

Along with Automattic/newspack-blocks#1682, fixes a regression introduced by #2866.

How to test the changes in this Pull Request:

  1. On release, set your Reader Revenue platform to NRH and disable WooCommerce.
  2. View a Donate block in the editor and on the front-end.
  3. Observe that the Frequency-based Donate block has only a single static option per frequency (no tiers) and no "other" number input. Similar to how it appears with Woo minus NYP:
Screenshot 2024-01-19 at 12 33 13 PM
  1. Observe that the Tiers-based Donate block shows "0" in all tiers in the editor.
  2. Check out this branch and fix: donate block layout for NRH newspack-blocks#1682, and confirm that the blocks appear more as expected:
Screenshot 2024-02-27 at 6 22 13 PM

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 dkoo added [Status] Needs Review The issue or pull request needs to be reviewed News Revenue Hub labels Feb 28, 2024
@dkoo dkoo self-assigned this Feb 28, 2024
@dkoo dkoo requested a review from a team as a code owner February 28, 2024 01:22
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

I took this for a test run, but not on a site that has NRH connected, just on a site set to use NRH as a donation platform.

Overall it looks good, but I ran into one issue: in the editor, the tiered layout of the block shows a $0 amount, even though it displays a proper dollar amount on the front end. The frequency view was fine and showed dollar amounts:

image

@dkoo
Copy link
Contributor Author

dkoo commented Feb 28, 2024

Overall it looks good, but I ran into one issue: in the editor, the tiered layout of the block shows a $0 amount, even though it displays a proper dollar amount on the front end.

@laurelfulford I pushed a fix for this in the other repo: abf4dd9

Also applies some CSS so that the look matches the Woo-based block more closely:

Screenshot 2024-02-28 at 3 08 13 PM

@dkoo
Copy link
Contributor Author

dkoo commented Feb 28, 2024

Whoops, looks like that last change broke things if the platform is set to NRH, but WooCommerce is active. Fixed that in 7fa0e66.

@laurelfulford laurelfulford self-requested a review February 28, 2024 22:42
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thanks @dkoo!

I'm just going to keep the feedback on this PR since it's easier to have it all in one place, though I think this is all still related to the block part of the changes.

Testing, I'm still getting a bit of weirdness in the editor:

  • Both on localhost and on the JN site, I'm getting the currency locale (US) in the editor (not on the front-end).
  • The buttons are also misaligned in the editor (sorry I missed that earlier!)
image

I also compared the block to how it used to look, and it's still not quite 1:1: the price should use the heading font as well (not the body) and be a bit bigger/bold; it should also be followed by the frequency amount:

image

When I switch back to Woo/NYP on this branch, some of these issues persist:

  • The font is wrong on the prices on the front-end and in the editor
  • The currency symbol is missing
  • In the editor, the button text is also left-aligned:

Woo/NYP front-end:
image

Just let me know if you have any questions, or if I can help with any fixes!

@laurelfulford laurelfulford self-requested a review February 29, 2024 01:24
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

I re-tested these this PR and the block PR with the following combinations, attempting to check out with each:

  • NRH as RR platform; Woo, NYP enabled
  • NRH as RR platform; Woo enabled
  • NRH as RR platform
  • Newspack as RR platform; Woo, NYP enabled
  • Newspack as RR platform; Woo enabled

... and compared them to a JN sites running a previous version. Outside of one small display issue I fixed (which existed before any of the NYP changes), things look good!

I also tested changing the button colours, and the theme's fonts -- both were picked up in expected ways with this PR.

The only thing I noticed is that if you don't have NYP enabled, you lose the ability to configure a block manually. Without tiers or custom pricing, this would only be the ability to enable/disable tabs on a specific block -- so if you have 'Once' turned off under Reader Revenue, you can't turn it on one an individual block. I think this could be changed as an enhancement to the non-NYP, non-NRH version of the block down the road if we wanted to -- it's not a regression.

Going to mark this and the related block PR as approved!

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Feb 29, 2024
@dkoo dkoo merged commit 8595985 into release Feb 29, 2024
12 checks passed
@dkoo dkoo deleted the hotfix/nrh-without-woo branch February 29, 2024 18:55
matticbot pushed a commit that referenced this pull request Feb 29, 2024
## [3.1.2](v3.1.1...v3.1.2) (2024-02-29)

### Bug Fixes

* donate block layout for NRH ([#2954](#2954)) ([8595985](8595985))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 29, 2024
# [3.2.0-alpha.2](v3.2.0-alpha.1...v3.2.0-alpha.2) (2024-02-29)

### Bug Fixes

* donate block layout for NRH ([#2954](#2954)) ([8595985](8595985))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.2.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
News Revenue Hub released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants