Skip to content

Conversation

@mcallegari10
Copy link
Contributor

@mcallegari10 mcallegari10 commented Apr 8, 2020

New features

Fixes

  • Fix addLinkSnippet type issue
  • Change prod configuration to export only the widget, not a minified version of React/ReactDOM

Other

  • Bump version to mark the 3.0 beta release
  • Remove the extension on styles files, change the .scss files to modules and change variables file to a folder

mcallegari10 added 2 commits April 5, 2020 20:09
- 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';

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';

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)

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]);

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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,

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?

Copy link
Contributor Author

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} />}

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} />?

Copy link
Contributor Author

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

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

Copy link
Contributor

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;

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) }
},

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) }
  }),

mcallegari10 added 3 commits April 9, 2020 17:52
- 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]);
Copy link
Contributor

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?

@mcallegari10 mcallegari10 merged commit 4705a23 into 3.0 Apr 14, 2020
@mcallegari10 mcallegari10 deleted the prs-features-refactor branch April 14, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants