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

allow edit of user name #328

Merged
merged 4 commits into from Oct 21, 2022
Merged

Conversation

vinmaster
Copy link
Contributor

Which Issue was this PR made for?

What is within the scope with this PR

Allow changing of user name by prompting user response and then send web socket event for the change and then all users in the room receive the event to change name in user context

@siberowl
Copy link
Collaborator

2022-10-16.21-05-28.mp4

Thanks for the awesome work! It looks great.
I just saw this bug where pressing a button on the bottom bar caused the name to be reset.

@Njima1572
Copy link
Collaborator

Njima1572 commented Oct 18, 2022

Thank you @vinmaster for finding and contributing to the project.
Since this project was not very well documented what goes where, I think there are some confusions on what exactly is needed to update things.
In this particular issue, the user information is shared among all the people through webRTC protocol, after the initial handshake, it was needed to be updated through src/classes/PeerProcessor.ts, the example can be found in pages/RoomPage/index.tsx under updateSelfUserInfo. (Sorry about addUserInfo being a bad naming, should probably be updateUserInfo)

const updateSelfUserInfo = (info) => {
const newInfo = { ...selfUserInfoRef.current, ...info };
selfUserInfoRef.current = newInfo;
setSelfUserInfo(newInfo);
addUserInfo(socket.id)(newInfo);
};

What I would do instead is to reuse updateSelfUserInfo and change name through this.

It would be some props passing involved if this were to happen, but feel free to create follow up issue to avoid this props passing too!

Thank you again for your work!

@siberowl
Copy link
Collaborator

siberowl commented Oct 18, 2022

Thank you @vinmaster for finding and contributing to the project. Since this project was not very well documented what goes where, I think there are some confusions on what exactly is needed to update things. In this particular issue, the user information is shared among all the people through webRTC protocol, after the initial handshake, it was needed to be updated through src/classes/PeerProcessor.ts, the example can be found in pages/RoomPage/index.tsx under updateSelfUserInfo. (Sorry about addUserInfo being a bad naming, should probably be updateUserInfo)

const updateSelfUserInfo = (info) => {
const newInfo = { ...selfUserInfoRef.current, ...info };
selfUserInfoRef.current = newInfo;
setSelfUserInfo(newInfo);
addUserInfo(socket.id)(newInfo);
};

What I would do instead is to reuse updateSelfUserInfo and change name through this.

It would be some props passing involved if this were to happen, but feel free to create follow up issue to avoid this props passing too!

Thank you again for your work!

I'm personally ok with keeping the editUserName but yeah I totally missed that.
@vinmaster We don't actually hold any value for userInfos (reducers are confusing 😭) so you need to pass in the current userInfo as a parameter to the reducer like we do with addUser. If just re-using addUser in the RoomPage seems easier like @Njima1572 suggested, that totally works for me too.

@vinmaster
Copy link
Contributor Author

I've updated to re-use addUserInfo. Not 100% sure if this is what you guys meant 😅

Copy link
Collaborator

@siberowl siberowl left a comment

Choose a reason for hiding this comment

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

Once you make these changes it should be ready to go! Sorry it got so confusing🙏

client/src/pages/RoomPage/index.tsx Outdated Show resolved Hide resolved
client/src/components/Sidebar/UserList/index.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@siberowl siberowl left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the awesome work and good luck with hacktoberfest!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Change username
3 participants