-
Notifications
You must be signed in to change notification settings - Fork 469
[New-feature] Messages deletion - Read/Unread #168
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
Conversation
- Add onTextInputChange new prop - Fix addLinkSnippet typing issue
- mark messages as read/unread - change badge component to handle automatically new messages received - change prod configuration to export only the widget - bump version to mark the 3.0 beta
dev/App.tsx
Outdated
| import React, { Component } from 'react'; | ||
|
|
||
| import { Widget, addResponseMessage, setQuickButtons, toggleMsgLoader } from '../index'; | ||
| import { Widget, addResponseMessage, setQuickButtons, toggleMsgLoader, addLinkSnippet, toggleWidget } from '../index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using toggleWidget? I don't see it on this file 🤔
| @@ -1,8 +1,10 @@ | |||
| import React, { useEffect, useRef } from 'react'; | |||
| import { useSelector, useDispatch } from 'react-redux'; | |||
| import { format } from 'date-fns'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using a lot of functions from date-fns? If not you can just install date-fns/format and reduce the size a lot 😄
| useEffect(() => scrollToBottom(messageRef.current), [messages]); | ||
|
|
||
| useEffect(() => { | ||
| console.log(showChat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
| scrollToBottom(messageRef.current); | ||
| if (showChat && badgeCount) dispatch(markAllMessagesRead()); | ||
| else dispatch(setBadgeCount(messages.filter((message) => message.unread).length)); | ||
| }, [messages]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this dependencies are okey? I see that you are using lots of external values like showChat , badgeCount , dispatch, setBadgeCount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this array is not about dependencies but when the values change, the effect will run. So I just need the effect to run when the messages are changed. If I add the showChat value for example, the badge will never be reset and will show me the badge count whenever I toggle the widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the badgeCount since I need it to run also there 😄
| const typing = useSelector((state: GlobalState) => state.behavior.messageLoader); | ||
| const dispatch = useDispatch(); | ||
| const { messages, typing, showChat, badgeCount } = useSelector((state: GlobalState) => ({ | ||
| messages: state.messages.messages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add here: messages: state.messages.messages || [] You can avoid the validation below for the map. It will also know that messages will always be a list. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not that necessary. The store initial state will ensure that it will always be at least an empty array. The validation on the JSX it's just for precaution 😄.
| <button type="button" className={cn('rcw-launcher', { 'rcw-hide-sm': showChat })} onClick={toggle}> | ||
| <Badge badge={badge} /> | ||
| <button type="button" className={cn('rcw-launcher', { 'rcw-hide-sm': showChat })} onClick={toggleChat}> | ||
| {!showChat && <Badge badge={badgeCount} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of showChat || <Badge badge={badgeCount} />?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change it or?
| export type BehaviorActions = ToggleChat | ToggleInputDisabled | ToggleMsgLoader; | ||
|
|
||
| export type MessagesActions = AddUserMessage | AddResponseMessage | AddLinkSnippet | RenderCustomComponent | DropMessages | HideAvatar; | ||
| export type MessagesActions = AddUserMessage | AddResponseMessage | AddLinkSnippet | RenderCustomComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have my doubts about this. What is the advantage of having one type MessagesActions that can have lots of interfaces. I think the | should be use when we have lots of items that go perfect in 2 or 3 groups of types so we can group them. Here i just see a group of lots of types that each of them represents only 1 action. I think this type MessagesActions should be removed and use the specific types for each action. If you have each specific type for each action the search will be easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of having this type is to describe the dispatch for each reduce with its action type.
I think that it's not necessary to explicitly type each action creator, that could be refactored.
| type: typeof MARK_ALL_READ; | ||
| } | ||
|
|
||
| export type BehaviorActions = ToggleChat | ToggleInputDisabled | ToggleMsgLoader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| [DELETE_MESSAGES]: (state: MessagesState, { count, id }) => { | ||
| if (id) return { ...state, messages: state.messages.filter(message => message.customId !== id) } | ||
| return { ...state, messages: state.messages.splice(state.messages.length - 1, count) } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[DELETE_MESSAGES]: (state: MessagesState, { count, id }) =>
({ ...state, messages: id ?
state.messages.filter(message => message.customId !== id) :
state.messages.splice(state.messages.length - 1, count) }
}),
- change date-fns import path - add dependency to useffect in Messages component - change messages reducer to ternary in delete messages handle
- add new functions to the main export - remove badge prop
- add align start to timestamp in response messages - fix production build to not add the devtools
| scrollToBottom(messageRef.current); | ||
| if (showChat && badgeCount) dispatch(markAllMessagesRead()); | ||
| else dispatch(setBadgeCount(messages.filter((message) => message.unread).length)); | ||
| }, [messages, badgeCount]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't showChat be a dependency here?
New features
Fixes
Other
.scssfiles to modules and change variables file to a folder