search

lukejacksonn / react-slack-clone

Complete chat application, built with Chatkit | by @lukejacksonn
https://pusher.com/chatkit
MIT License
1.35k stars 262 forks source link

Changing Room Name/Deletion after Creation #32

Open EduardoSaverin opened 6 years ago

EduardoSaverin commented 6 years ago

Enhancement: Changing Room Name/Deletion once it is created.

MuhammadSaadQadeer commented 6 years ago

Hi @lukejacksonn I would like to give it a try , is anyone here on actively adding contributions to this project? I am a first timer here , so would might need a bit of help.

lukejacksonn commented 6 years ago

Hey 👋🏞I have been busy at work with other projects recently so there hasn't been much contribution from Pusher for a while but this would be cool to see and shouldn't be too hard..

I had envisioned putting contenteditable on the room name in the room-header component and then listening for a change and then calling updateRoom.

Hopefully this is enough to get you going. Feel free to PR for review!

MuhammadSaadQadeer commented 6 years ago

Hey Yes, thanks I am on it, I'll keep you posted.

MuhammadSaadQadeer commented 6 years ago

Hey, @lukejacksonn I have been working on this thing, I tried with content contenteditable but I couldn't figure out how you would listen for the change when the text changes inside the div element, also we need a button or some kind of control to check if user has done finishing the editing.

I tried another solution but it needs UI upgrades, it works like this, when a user clicks the title of the chat in the header, it will convert into a text field with a button spanned on the right, refer to the attachment. I have another solution in mind as well , if you want I can discuss that too. I am waiting for your feedback on this one.

sample

lukejacksonn commented 6 years ago

This looks like a reasonable approach! If you took some of the styling off of the input, then it would appear like a content editable div anyway. What is your other solution?

MuhammadSaadQadeer commented 6 years ago

The other solution would be to go with a modal, we can show a little edit icon with the title header and when the user clicks it a modal will be opened showing a text field and a done button. What do you say ?

Also I have another question regarding usage of any front end styling libraries like Bootstrap or material? You haven't added any of it. Using the built in components of such libraries would be handy.

lukejacksonn commented 6 years ago

I'd prefer not to be so intrusive. In place editing is much cleaner, it would be nice to be able to implement this feature with next to no extra UI at all. So I prefer your first solution.

That is generally my goal with all things programming; keep everything to a minimum. This kind of explains why I have not employed a CSS framework. This is not to say the CSS in this project is perfect by any means. But it is all that is required and I'd like to keep it that way for now!

MuhammadSaadQadeer commented 6 years ago

Okay sure, sounds good then , I'll complete this task asap and submit a PR for review. Thanks for the response 👍

MuhammadSaadQadeer commented 6 years ago

@lukejacksonn query I am getting this error (refer to the screen shot attached). Although I am the one who created the room what I am missing here.

lukejacksonn commented 6 years ago

Humm. Ohh yes, now I remember why I didn't implement this in the first place! So when you create an instance, two roles are created; default and admin which have different permissions.

All users are given the default role when they are created. This role lacks the room:update permission, see here:

image

So you would need to assign your user the admin role in order to update rooms. This will be much easier when we release our new and improved Instance Inspector in the dashboard. You could wait until we have released that or try doing it programatically, see here https://docs.pusher.com/chatkit/reference/roles-and-permissions#setting-a-user-role.

We are still trying to iron this process out, so please bare with us and let us know how you get on!

MuhammadSaadQadeer commented 6 years ago

Okay, I think for now I can go with the programmatic approach and complete the update name functionality. And when you guys will straight it out. The update name functionality would work as per my understanding.

MuhammadSaadQadeer commented 6 years ago

@lukejacksonn I cant seem to get my head around this, I have been trying to change my roles using programmatic approach but I am unable to send the PUT request for changing roles. Here's how I was doing it using CURL.

 curl -X PUT \ https://us1.pusherplatform.io/services/chatkit_authorizer/v1/instance_id:05f46048-3763-4482-9cfe-51ff32
7c3f29/users/user_id:MuhammadSaadQadeer/roles \ -d '{"name": "Admin"}'

But it gives me an error like this "error":"service_instance_id_invalid","error_description":"Service instance ID:instance_idcontains invalid symbols"

What I am doing wrong here , help me out.

lukejacksonn commented 6 years ago

Try to omit the string instance_id: and user_id: from the URL so that it looks more like this 

curl -X PUT \ https://us1.pusherplatform.io/services/chatkit_authorizer/v1/05f46048-3763-4482-9cfe-51ff327c3f29/users/MuhammadSaadQadeer/roles \ -d '{"name": "Admin"}'
MuhammadSaadQadeer commented 6 years ago

@lukejacksonn I tried what you have said, but I was raised with another issue, refer to the screen shot below.

problem Apart from that I have completed the UI functionality for updating the chat room name. I have submitted the PR kindly review the code and let me know if it is good to be merged in to the master or any updates are required. Currently it still gives the error of permission so for now I am displaying the error message in alert. Please have a look and update me. Thanks

lukejacksonn commented 6 years ago

Hey @MuhammadSaadQadeer I have been busy working on the instance inspector chatkit and have got it to the point where we can edit permission on the fly. You were doing everything correctly, the permission was not included for your user!

So I have added the update:room permission to the default role which is assigned to your user on the app. Which means now when you are testing the feature, you should not see a permissions error.

The code in this PR looks good but I haven't had time to properly review it yet. In the meantime if you could let me know if you are allowed to update the name of a room then that would be good to know!

MuhammadSaadQadeer commented 6 years ago

Hey @lukejacksonn I have tested it the room gets updated successfully , while I was testing I needed to change some of the code again to make it right. So I have the latest code on my branch, I am going to commit push the change. Do I have to create another PR ?

lukejacksonn commented 6 years ago

Great news! If you push up the same branch, it will update the first PR you made but it looks like you figured that out already 🎉 I will try review it asap.

MuhammadSaadQadeer commented 6 years ago

Yeah thanks for the guiding me along as this is my first contribution. It helped me a lot to learn. Let me know about the feedback when you review it or any other thing you need me for.