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 comments block #7724

Merged
merged 32 commits into from
Feb 19, 2025

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Feb 12, 2025

Resolves: GIVE-1394

Description

Reference: Zeplin

This add the Campaign Comments block.

  • New route registered: GetCampaignComments.php.

Visuals

comments.mov

Testing Instructions

  • Create a campaign.
  • Add block & test out the controls.
  • Anonymous donations can be filtered out and should not show any personal information if displayed.
  • Empty Comments do not show.
  • Avatar(Gravatar) can be hidden.
  • Name can be hidden.
  • Date can be hidden.
  • Comment length can be truncated.
  • Title can be edited.
  • Read more button text can be edited.
  • Read more button should not show if actual comment is less then the set comment length.
  • Amount of comments to be shown per-page can be chosen.

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

@JoshuaHungDinh JoshuaHungDinh changed the base branch from develop to epic/campaigns February 12, 2025 07:39
Copy link
Member

@alaca alaca left a comment

Choose a reason for hiding this comment

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

Hey @JoshuaHungDinh
I hope you don't mind me checking your PR. I left a comment regarding assets loading.


$encodedAttributes = json_encode($blockAttributes->toArray());

$this->loadScripts($blockAttributes);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of loading assets on the backend, this is a new approach we are using now (block.json). With this approach you don't need loadScripts method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey yea thanks, I've been having difficulties localizing the window without registering it here. Any suggestions or thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Hey Joshua, I'd suggest registering a new route - /campaign/(?P<id>\d+)/comments and then fetch data from the block app. For data fetching you can use useSWR which is great. Here is an example of how to use it

const campaignForms = (() => {

Also, the current approach might not be the best if we want to introduce pagination for comments. Imagine there are hundreds of comments, which is unlikely but, the current approach won't be that efficient because all comments data will be loaded every single time. And if there are multiple blocks on the same page, but from different campaigns, every single one of them will use the same global object GiveCampaignCommentsBlockWindowData.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alaca I think that's a good suggestion for now, however in the future we should consider using the donations endpoint to get the comments (when those endpoints are ready). We can then filter by campaignId and HAL to include donor data, checkout the slack thread on this topic https://lw.slack.com/archives/C04SLRDD9CK/p1738879925746179

@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review February 13, 2025 11:12
@JoshuaHungDinh JoshuaHungDinh requested a review from alaca February 14, 2025 04:44
@JoshuaHungDinh
Copy link
Contributor Author

Thanks @jonwaldstein I also cleaned up the empty state UI . 3e625cd

Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

@JoshuaHungDinh nice job man!

@jonwaldstein jonwaldstein merged commit b529388 into epic/campaigns Feb 19, 2025
20 checks passed
@jonwaldstein jonwaldstein deleted the feature/campaign-comments-block-GIVE-1394 branch February 19, 2025 15:34
@JoshuaHungDinh JoshuaHungDinh removed the request for review from alaca February 19, 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.

4 participants