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

Feature: Add Campaign Donations block #7703

Open
wants to merge 6 commits into
base: feature/campaign-donors-block
Choose a base branch
from

Conversation

pauloiankoski
Copy link
Contributor

@pauloiankoski pauloiankoski commented Jan 31, 2025

Resolves GIVE-1393

Description

This pull request adds the Campaign Donations block, with options to show the top donations (by amount) or the recent donation (by date). This block is like a copy of the Campaign Donors block with some slightly changes on the visual but mainly in its query.

Visuals

CleanShot 2025-01-31 at 08 35 34
Block Settings

CleanShot 2025-01-31 at 08 34 26
Top donations and Recent donations

CleanShot 2025-01-31 at 09 40 31
Empty state

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@pauloiankoski pauloiankoski requested a review from alaca January 31, 2025 12:41
/**
* @unreleased
*/
private function renderBlockHtml($donations): void
Copy link
Member

Choose a reason for hiding this comment

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

Oh man, I'm having a hard time reading this 😅
Why don't you move this into a separate file and then use the View class to load and render the file?

Copy link
Member

Choose a reason for hiding this comment

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

This is a very WP way of doing these things and I have to say that I'm not a big fan 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? I even tried to make it more object oriented, moving away from WP way of doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss this more.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't like to mix PHP and HTML. I would love to see HTML in a separate file, and then you load it with the View helper class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jon would like to see the template as being a React app.

/**
* @unreleased
*/
if ( ! class_exists(CampaignDonationsBlockRenderer::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to check if the class exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I didn't go much further on that but when in the Page Editor, I was receiving a fatal error from the block renderer saying that class was being registered multiple times.

Copy link
Member

Choose a reason for hiding this comment

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

This is because the CampaignDonationsBlockRenderer is defined in the render.php file, and this file is included each time you include this block. The solution would be to move the class definition outside the render.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored on 9392c10

@pauloiankoski pauloiankoski force-pushed the feature/campaign-donations-block branch from 90390f7 to 4977ca5 Compare January 31, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants