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.

Sign up for GitHub

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

Communication: Update group postings in-place #10283

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+49 −31
Open

Communication: Update group postings in-place #10283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 49 additions & 31 deletions ...view/course-conversations/layout/conversation-messages/conversation-messages.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,53 +258,70 @@ export class ConversationMessagesComponent implements OnInit, AfterViewInit, OnD
return;
}

// Separate pinned posts into their own group
// Separate pinned posts from others.
const pinnedPosts = this.posts.filter((post) => post.displayPriority === DisplayPriority.PINNED);
const unpinnedPosts = this.posts.filter((post) => post.displayPriority !== DisplayPriority.PINNED);

// Sort unpinned posts ascending by creation date.
const sortedPosts = unpinnedPosts.sort((a, b) => {
const aDate = (a as any).creationDateDayjs;
const bDate = (b as any).creationDateDayjs;
return aDate?.valueOf() - bDate?.valueOf();
badkeyy marked this conversation as resolved.
Show resolved Hide resolved
});

const groups: PostGroup[] = [];
let currentGroup: PostGroup = {
author: sortedPosts[0].author,
posts: [{ ...sortedPosts[0], isConsecutive: false }],
};

for (let i = 1; i < sortedPosts.length; i++) {
const currentPost = sortedPosts[i];
const lastPostInGroup = currentGroup.posts[currentGroup.posts.length - 1];
// Build updated groups for non-pinned posts.
const updatedGroups: PostGroup[] = [];
if (sortedPosts.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (sortedPosts.length > 0) {
if (sortedPosts.length) {

let currentGroup: PostGroup = {
author: sortedPosts[0].author,
posts: [{ ...sortedPosts[0], isConsecutive: false }],
};
for (let i = 1; i < sortedPosts.length; i++) {
const currentPost = sortedPosts[i];
const lastPostInGroup = currentGroup.posts[currentGroup.posts.length - 1];
const currentDate = (currentPost as any).creationDateDayjs;
const lastDate = (lastPostInGroup as any).creationDateDayjs;
let timeDiff = Number.MAX_SAFE_INTEGER;
if (currentDate && lastDate) {
timeDiff = currentDate.diff(lastDate, 'minute');
}
Comment on lines +284 to +287
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let timeDiff = Number.MAX_SAFE_INTEGER;
if (currentDate && lastDate) {
timeDiff = currentDate.diff(lastDate, 'minute');
}
const timeDiff = currentDate && lastDate ? currentDate.diff(lastDate, 'minute') : Number.MAX_SAFE_INTEGER;

if (currentPost.author?.id === currentGroup.author?.id && timeDiff < 5 && timeDiff >= 0) {
currentGroup.posts.push({ ...currentPost, isConsecutive: true });
} else {
updatedGroups.push(currentGroup);
currentGroup = {
author: currentPost.author,
posts: [{ ...currentPost, isConsecutive: false }],
};
}
}
updatedGroups.push(currentGroup);
}

const currentDate = (currentPost as any).creationDateDayjs;
const lastDate = (lastPostInGroup as any).creationDateDayjs;
if (pinnedPosts.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (pinnedPosts.length > 0) {
if (pinnedPosts.length) {

updatedGroups.unshift({ author: undefined, posts: pinnedPosts });
}

let timeDiff = Number.MAX_SAFE_INTEGER;
if (currentDate && lastDate) {
timeDiff = currentDate.diff(lastDate, 'minute');
}
const groupsEqual = (groupA: PostGroup, groupB: PostGroup): boolean => {
if (!groupA.author && !groupB.author) return true;
if (!groupA.author || !groupB.author) return false;
return groupA.author.id === groupB.author.id;
};
Comment on lines +305 to +309
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you transform it into a function and simplify it? Please rename it something the function is actually doing.

function isAuthorEqual(groupA: PostGroup, groupB: PostGroup): boolean {
    // Both groups are equal if neither has an author; otherwise, they are not
    if (!groupA.author || !groupB.author) {
        return !groupA.author && !groupB.author;
    }
    return groupA.author.id === groupB.author.id;
}


if (currentPost.author?.id === currentGroup.author?.id && timeDiff < 5 && timeDiff >= 0) {
currentGroup.posts.push({ ...currentPost, isConsecutive: true }); // consecutive post
for (let i = 0; i < updatedGroups.length; i++) {
if (this.groupedPosts[i] && groupsEqual(this.groupedPosts[i], updatedGroups[i])) {
this.groupedPosts[i].posts = updatedGroups[i].posts;
} else {
groups.push(currentGroup);
currentGroup = {
author: currentPost.author,
posts: [{ ...currentPost, isConsecutive: false }],
};
if (this.groupedPosts[i]) {
this.groupedPosts[i] = updatedGroups[i];
} else {
this.groupedPosts.push(updatedGroups[i]);
}
}
}

groups.push(currentGroup);

// Only add pinned group if pinned posts exist
if (pinnedPosts.length > 0) {
groups.unshift({ author: undefined, posts: pinnedPosts });
while (this.groupedPosts.length > updatedGroups.length) {
this.groupedPosts.pop();
}
Comment on lines +311 to 324
Copy link
Contributor

Choose a reason for hiding this comment

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

It's super hard to grasp what's happening in here.
Can you please improve this? Please use forEach instead of for(...) since there is no need for the for loop. Also, refactor it to avoid the nested if and else structure and avoid using the while loop. Feel free to ping me on Slack, and I'll discuss some ideas for improvement with you.


this.groupedPosts = groups;
this.cdr.detectChanges();
}

Expand All @@ -321,6 +338,7 @@ export class ConversationMessagesComponent implements OnInit, AfterViewInit, OnD
return post;
});

// Incrementally update the grouped posts.
this.groupPosts();
}

Expand Down
Loading