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

feat: add today message divider #67

Merged
merged 1 commit into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 19 additions & 2 deletions src/components/ChatBox/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,32 @@ import { useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import Message from '../Message';
import './ChatBox.scss';
import MessageDivider from '../MessageDivider';

function isToday(date) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not a blocking feedback. That said, why can't a open source library like moment.js help you here?

Copy link
Member

@rijuma rijuma Nov 22, 2024

Choose a reason for hiding this comment

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

IMHO, I'm not fully agree on adding a library to the bundle and dependencies just for something trivial, but I understand the point.

If we don't add the dependency, we might want to compare dates as strings to make it a bit simpler.

const now = new Date()
const isToday = (now.toDateString() === otherDate.toDateString());

Just leaving it as an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you both! So part of the issue with comparing the dates as strings is that these are timestamps. So I would need to extract the date only first and compare those strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Simon, to your point, I did look in frontend-app-learning to see if something similar was implemented and I did see this: https://github.com/openedx/frontend-app-learning/blob/master/src/course-home/dates-tab/utils.jsx. I wonder if there was a reason that we didn't just use another library there; I wonder if for a similar reason as what Marcos suggested?

Copy link
Member

Choose a reason for hiding this comment

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

This is how custom code proliferate. Some engineer think the custom code at that time is a good idea, then it gets accepted. Others later would find it to be acceptable too. I get it. This is why my question here is not a blocking feedback.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true. I'll do some quick research on moment.js and make the call soon to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay so I went to moment.js and see an article suggesting more modern alternatives: https://momentjs.com/. Maybe for times sake I'll move forward as is though this isn't the ideal. Thanks for the feedback in any case.

const today = new Date();
return (
date.getDate() === today.getDate()
&& date.getMonth() === today.getMonth()
&& date.getFullYear() === today.getFullYear()
);
}

// container for all of the messages
const ChatBox = ({ chatboxContainerRef }) => {
const { messageList, apiIsLoading } = useSelector(state => state.learningAssistant);
const messagesBeforeToday = messageList.filter((m) => (!isToday(new Date(m.timestamp))));
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the logic here to ensure that the divider only shows for the conditions below.

const messagesToday = messageList.filter((m) => (isToday(new Date(m.timestamp))));

// message divider should not display if no messages or if all messages sent today.
return (
<div ref={chatboxContainerRef} className="flex-grow-1 scroller d-flex flex-column pb-4">
{messageList.map(({ role, content, timestamp }) => (
<Message key={timestamp.toString()} variant={role} message={content} />
{messagesBeforeToday.map(({ role, content, timestamp }) => (
<Message key={timestamp} variant={role} message={content} />
))}
{(messageList.length !== 0 && messagesBeforeToday.length !== 0) && (<MessageDivider text="Today" />)}
{messagesToday.map(({ role, content, timestamp }) => (
<Message key={timestamp} variant={role} message={content} />
))}
{apiIsLoading && (
<div className="loading">Xpert is thinking</div>
Expand Down
102 changes: 102 additions & 0 deletions src/components/ChatBox/index.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import React from 'react';
import { screen, act } from '@testing-library/react';

import { render as renderComponent } from '../../utils/utils.test';
import { initialState } from '../../data/slice';

import ChatBox from '.';

const mockDispatch = jest.fn();
jest.mock('react-redux', () => ({
...jest.requireActual('react-redux'),
useDispatch: () => mockDispatch,
}));

const defaultProps = {
chatboxContainerRef: jest.fn(),
};

const render = async (props = {}, sliceState = {}) => {
const componentProps = {
...defaultProps,
...props,
};

const initState = {
preloadedState: {
learningAssistant: {
...initialState,
...sliceState,
},
},
};
return act(async () => renderComponent(
<ChatBox {...componentProps} />,
initState,
));
};

describe('<ChatBox />', () => {
beforeEach(() => {
jest.resetAllMocks();
});
it('message divider does not appear when no messages', () => {
const messageList = [];
const sliceState = {
messageList,
};
render(undefined, sliceState);

expect(screen.queryByText('Today')).not.toBeInTheDocument();
});

it('message divider does not appear when all messages from today', () => {
const date = new Date();
const messageList = [
{ role: 'user', content: 'hi', timestamp: date - 60 },
{ role: 'user', content: 'hello', timestamp: date },
];
const sliceState = {
messageList,
};
render(undefined, sliceState);

expect(screen.queryByText('hi')).toBeInTheDocument();
expect(screen.queryByText('hello')).toBeInTheDocument();
expect(screen.queryByText('Today')).not.toBeInTheDocument();
});

it('message divider shows when all messages from before today', () => {
const date = new Date();
const messageList = [
{ role: 'user', content: 'hi', timestamp: date.setDate(date.getDate() - 1) },
{ role: 'user', content: 'hello', timestamp: date + 1 },
];
const sliceState = {
messageList,
};
render(undefined, sliceState);

expect(screen.queryByText('hi')).toBeInTheDocument();
expect(screen.queryByText('hello')).toBeInTheDocument();
expect(screen.queryByText('Today')).toBeInTheDocument();
});

it('correctly divides old and new messages', () => {
const today = new Date();
const messageList = [
{ role: 'user', content: 'Today yesterday', timestamp: today.setDate(today.getDate() - 1) },
{ role: 'user', content: 'Today today', timestamp: +Date.now() },
];
const sliceState = {
messageList,
};
render(undefined, sliceState);

const results = screen.getAllByText('Today', { exact: false });
expect(results.length).toBe(3);
expect(results[0]).toHaveTextContent('Today yesterday');
expect(results[1]).toHaveTextContent('Today');
expect(results[2]).toHaveTextContent('Today today');
});
});
20 changes: 20 additions & 0 deletions src/components/MessageDivider/MessageDivider.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@use '../../utils/variables';
varshamenon4 marked this conversation as resolved.
Show resolved Hide resolved

.message-divider {
display: flex;
font-size: 15px;
}
.message-divider:before, .message-divider:after{
content: "";
flex: 1 1;
border-bottom: 1px solid;
margin: auto;
varshamenon4 marked this conversation as resolved.
Show resolved Hide resolved
}
.message-divider:before {
margin-right: 10px;
margin-left: 10px;
}
.message-divider:after {
margin-right: 10px;
margin-left: 10px;
}
15 changes: 15 additions & 0 deletions src/components/MessageDivider/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React from 'react';
import './MessageDivider.scss';
import PropTypes from 'prop-types';

const MessageDivider = ({ text }) => (
<div className="message-divider">
{text}
</div>
);

MessageDivider.propTypes = {
text: PropTypes.string.isRequired,
};

export default MessageDivider;