pitaj / nodebb-plugin-calendar

Fully-featured calendar plugin for NodeBB
42 stars 34 forks source link

feat: add participation counter to event displays #154

Closed Frozen-byte closed 1 year ago

Frozen-byte commented 1 year ago

This MR adds a number behind the response-list headers to indicate how many responses were given:

image image
Frozen-byte commented 1 year ago

I think the counts should be added server-side instead.

Add a getCounts function (takes an array of pids) to the responses API and call that in each of the applicable event APIs.

You'll also need to add a responseCounts property to the Event type or maybe EventWithCid.

Then you can edit the template to put those properties directly into the HTML.

The other responses stuff is done client side because it's user-specific, and parsed posts are cached across users. This isn't user specific, so it's best to do it server side instead.

Responses are also loaded on demand currently, and this would change that to load them when the page or post is loaded instead. If we wanted to do that instead, then a better place to do would be in setupPost. But I maintain it should be done server side instead.

I totally agree, rendering the initial count server sided (and maybe taking care of live attendee changes via listening to a webhook)!

I initially tried that approach, but struggled a lot fetching the correct data in eventTemplate().
Haven't thought of saving the actual count in the DB and mutating it on every submitResponse call, that should have the greatest performance I think.

My failed approach was:
The passed argument for eventTemplate() does not have any responses loaded, nor a eventPid to load them on demand.
This is because the parsePost() function only receives the text event from the actual post, which does not include the eventPid.

pitaj commented 1 year ago

tried that approach, but struggled a lot fetching the correct data in eventTemplate(). Haven't thought of saving the actual count in the DB and mutating it on every submitResponse call, that should have the greatest performance I think.

Yeah eventTemplate is not the place to do that. We may want to optimize by adding explicit counts to the database in the future. But for now, just use setsCount

You'll notice that responses::getAll takes a day parameter. Since you won't have that, I'd start out by only showing response counts if the event doesn't repeat.

Frozen-byte commented 1 year ago

I notice one UX flaw, that is more prominent with the numbers: Clicking the response button does not update the attendee list, nor the count. I don't think that should be fixed in this PR, and there is a more general approach to update the calendar-event based on a socket-event. Will open a Ticket for it: https://github.com/pitaj/nodebb-plugin-calendar/issues/155

pitaj commented 1 year ago

Just fyi I may not get this merged for a week or so. Need to make time to really go over it on my computer.

Thanks