-
Notifications
You must be signed in to change notification settings - Fork 195
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
Feature: add campaign comments block #7724
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
Thanks @jonwaldstein I also cleaned up the empty state UI . 3e625cd |
There was a problem hiding this 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!
Resolves: GIVE-1394
Description
Reference: Zeplin
This add the Campaign Comments block.
GetCampaignComments.php
.Visuals
comments.mov
Testing Instructions
Pre-review Checklist
@unreleased
tags included in DocBlocks