feat: add today message divider #67
Conversation
|
|
||
| // 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)))); |
There was a problem hiding this comment.
Updated the logic here to ensure that the divider only shows for the conditions below.
|
One question (and maybe this could be cleared up in a demo), but the image included in the PR shows that the last message is hidden behind the message text field. Is the auto scroll behavior not working as expected now? I would expect the message to not be hidden behind the field. Thanks! |
@alangsto ah yes! This is just because I had to scroll up to see the divider. It does still auto scroll as expected. Happy to demo to show how it works or even just add a screenshare. |
varshamenon4
force-pushed
the
varshamenon4/feat-add-old-message-divider
branch
from
0db591c
to
baa26ef
Compare
| @@ -3,15 +3,34 @@ import { useSelector } from 'react-redux'; | |||
| import PropTypes from 'prop-types'; | |||
| import Message from '../Message'; | |||
| import './ChatBox.scss'; | |||
| import MessageDivider from '../MessageDivider'; | |||
|
|
|||
| function isToday(date) { | |||
There was a problem hiding this comment.
This is not a blocking feedback. That said, why can't a open source library like moment.js help you here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's true. I'll do some quick research on moment.js and make the call soon to merge.
There was a problem hiding this comment.
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.
varshamenon4
force-pushed
the
varshamenon4/feat-add-old-message-divider
branch
from
1b16b12
to
d5019ab
Compare
COSMO-544
This PR adds the Today message divider which divides old and new messages. See screenshot below: