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

Added ability to have multiple chats #194

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

eduardsmetanin
Copy link

@mattmezza Please let me know if you have any questions or want to discuss. I updated the the chat and the demo, but not your doc. I can do it if necessary. The old demo works with the new chat, so the changes are backward compatible.

@eduardsmetanin
Copy link
Author

eduardsmetanin commented Oct 12, 2020

@mattmezza Hi Matteo! It looks like mine and several other pull requests failing checks with error "Error: Resource not accessible by integration". It seems to be only happening right after message "Adding message: Thanks for sending out your first PR! to pull request". Could you take a look? https://github.com/mattmezza/vue-beautiful-chat/pull/194/checks?check_run_id=1239683641 The issue is discussed here actions/first-interaction#10 Could you remove this action?

Copy link
Owner

@mattmezza mattmezza left a comment

Choose a reason for hiding this comment

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

Hi @eduardsmetanin and thanks for this great contribution! This brings a cool feature and I'm excited to merge it asap. I only have some minor comments that I would like to tackle before merging.
Also, I have been merging some new addition and it would be great to rebase on master 👍

@a-kriya had an idea to further improve this library be pushing some of the props that are now propagated to deeply nested components back into the store. This would simplify the setup even more (see #178).

Cheers,
Matt.

src/ChatList.vue Outdated Show resolved Hide resolved
src/ChatWindow.vue Show resolved Hide resolved
src/ChatWindow.vue Outdated Show resolved Hide resolved
src/Launcher.vue Outdated Show resolved Hide resolved
@eduardsmetanin
Copy link
Author

@mattmezza I'm done with my fixes. Please take a look again. As for @a-kriya 's idea, why don't you merge it? It would be easier for contributors to see it and do the same. Right now I don't feel like converting my code to this style within this already large pull request.

@a-kriya
Copy link
Collaborator

a-kriya commented Oct 21, 2020

@eduardsmetanin It's been merged. Since there are now conflicts with master, you'll have to rebase and resolve them. And using the store to access the necessary props will reduce the size of the diff.

@eduardsmetanin
Copy link
Author

@mattmezza @a-kriya This pull request is rebased and ready to be merged. I tried to use store to pass props, but I ran into 2 issues and rolled back store.state my changes:

  • A prop is undefined when accessed from a method called from data() and this prop is populated via ...mapState(). Perhaps initial value can be set like this:
    const store = { state: Vue.observable({ editMessage: null }),, but I ran into a case when I needed my data (initialized in data()) to depend on a prop, which didn't work because of undefined value in the prop. As a workaround this prop can be accessed via store.state['my_prop_name']. This caused me a lot of time and frustration. Vue props work well in this case and I think we need a state management system that works just like standard props.
  • We currently update entire state map on every prop change:
    handler(props) { // TODO: optimize for (const prop in props) { store.setState(prop, props[prop]) } }. The TODO says it needs to be optimized. I don't know how it can be optimized.

Does Vue 3 provide an easier way to manage state? Perhaps we should wait for Vue 3 conversion and then change how we manage state?

demo/src/App.vue Outdated
Comment on lines 266 to 277
const chat = this.chatHistory[this.currentChatID];
if (chat === undefined) {
return [];
}
return chat["messages"];
},
participants() {
const chat = this.chatHistory[this.currentChatID];
if (chat === undefined) {
return [];
}
return chat["participants"];
Copy link
Collaborator

@a-kriya a-kriya Oct 22, 2020

Choose a reason for hiding this comment

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

Parts of this diff has semi-colons and uses double-quotes instead of single. Can you please run npm run lint and update this PR? It should fix most style issues automatically.
Why access object props using subscripts rather than a period?

Comment on lines +64 to +66
multipleChatsEnabled: {
type: Boolean,
required: true
Copy link
Collaborator

@a-kriya a-kriya Oct 22, 2020

Choose a reason for hiding this comment

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

Why is this a prop? Is there any difference when this is off? I would expect a single instance of the component to work just as it has, and multiple instances to also work out-of-the-box. I can't see why it needs to be configured.

Copy link
Author

Choose a reason for hiding this comment

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

In multi-room mode, when chat window is opened, I show list of chat rooms (as opposite to chat right away). Also, in the header for chat window I show go-back-to-chat-list button. I use multipleChatsEnabled prop to determine if those 2 things need to be done. I don't see any other good way to do it. The main reason for that is the demo app is controlling the chat content and there is only one property (messageList) with chat content it passes launcher. So, the demo app puts correct chat content when chat room changes. I considered to use chatList prop instead of multipleChatsEnabled (if it's empty or undefined than assume single chat mode), but it's possible to have 0 chat rooms in multi-chat mode.

@silasrm
Copy link

silasrm commented Nov 27, 2020

Hi guys,

Any news about this PR? I'm finding this feature.
@eduardsmetanin if I clone your repo, this is 100% ok?
Doc in your repo is outdated!

@eduardsmetanin
Copy link
Author

Hi guys,

Any news about this PR? I'm finding this feature.
@eduardsmetanin if I clone your repo, this is 100% ok?
Doc in your repo is outdated!

@silasrm You can use my clone if you want. I do NOT promise to maintain it though. I haven't updated any docs yet. It would be good to see a will to merge my changes from maintainer @mattmezza before I spend any more time on it.

@silasrm
Copy link

silasrm commented Nov 28, 2020

Hi guys,
Any news about this PR? I'm finding this feature.
@eduardsmetanin if I clone your repo, this is 100% ok?
Doc in your repo is outdated!

@silasrm You can use my clone if you want. I do NOT promise to maintain it though. I haven't updated any docs yet. It would be good to see a will to merge my changes from maintainer @mattmezza before I spend any more time on it.

I'm using after clone and build. Now, I'm trying to integrate to https://github.com/musonza/chat :D

@silasrm
Copy link

silasrm commented Nov 30, 2020

@eduardsmetanin I add support to toolbox on emoji and file message types.
beefreelancer#1

@miloandco
Copy link

@mattmezza Could you please merge this pull request?

@miloandco
Copy link

@eduardsmetanin Could you maybe solve the conflict so that your pull request can be merged in? :)

@eduardsmetanin
Copy link
Author

@a-kriya Will you merge this pull request if I resolve conflicts? People ask for my changes once in a while, so I think it would be beneficial for the project. I can walk you through my changes via a conference call if you wish.

Attn: @miloandco @oldner

@eduardsmetanin Could you maybe solve the conflict so that your pull request can be merged in? :)

@stale
Copy link

stale bot commented May 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 15, 2021
@a-kriya a-kriya removed the stale label May 16, 2021
@stale
Copy link

stale bot commented Jul 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 15, 2021
@a-kriya a-kriya removed the stale label Jul 15, 2021
@stale
Copy link

stale bot commented Nov 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 6, 2021
@a-kriya a-kriya added the pinned label Nov 6, 2021
@stale stale bot removed the stale label Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants