Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

Conversation

@janzenisaac
Copy link
Contributor

@janzenisaac janzenisaac commented Apr 25, 2025

Preview

scroll.with.date.mov

Details

  • Group pms by last_posted_at. In this first iteration we are group by 7 days, 30 days, then by month beyond that.
  • I inject a sidebar section link with the relative (last_posted_at) date and then update a tracked value to ensure we don't do it again. Then for each month beyond the first 30days, I add a value to the loadedMonthLabels set and we reference that (plus the year) to see if we need to load a new month label.
  • I took the creative liberty to remove the Conversations section label - this had no purpose
  • I hid the collapse all sidebar sections carrot. This had no purpose.
  • Swap BasicTopicSerializer to ListableTopicSerializer to get access to last_posted_at

Copy link
Member

@keegangeorge keegangeorge left a comment

Choose a reason for hiding this comment

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

Looks good, just one small suggested change

render json: {
conversations: serialize_data(pms, BasicTopicSerializer),
conversations: serialize_data(pms, ListableTopicSerializer),
meta: {
Copy link
Member

Choose a reason for hiding this comment

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

If all we need is last_posted_at should we instead extend BasicTopicSerializer and do something like:

class BotConversationTopicSerializer < BasicTopicSerializer
  attributes :last_posted_at
end

since ListableTopicSerializer looks like it's adding a wealth of attributes we don't really need, so doing this might keep it lightweight and avoid unnecessary db calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could... this is so light weight (overall) and we have to carry the extra extension. I think the play is to stick with Listable vs extending. Are you okay to merge as is?

Copy link
Member

@keegangeorge keegangeorge Apr 25, 2025

Choose a reason for hiding this comment

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

Yeah that makes sense, you're right, it's still pretty lightweight and is probably better than carrying the extra extension. ✅ Good to merge

@janzenisaac janzenisaac merged commit cd0cfc0 into main Apr 25, 2025
6 checks passed
@janzenisaac janzenisaac deleted the group-pms-by-date branch April 25, 2025 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants