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.

Sign up for GitHub

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

Jump to bottom

Add chat to game-event page #96

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Add chat to game-event page #96

merged 1 commit into from
Jun 6, 2023

Conversation

MaayanMashhadi
Copy link
Collaborator

@MaayanMashhadi MaayanMashhadi commented May 27, 2023 •
edited
Loading

Desired Outcome

Players will be able to chat with each other in the specific event. Only the players
who joined the event, will be able to enter the chat.

Implemented Changes

  1. modify game-event html for adding chat html
  2. modify game-event view to present the messages
  3. adding new tests
  4. adding new view function for chat
  5. adding new url for the view function

Connected Issue/Story

Resolves #95

Dependencies

  • This PR does not depend on other PR's

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes

Documentation

  • This PR does not require updating any documentation

@MaayanMashhadi MaayanMashhadi marked this pull request as draft May 27, 2023 12:41
@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 3 times, most recently from 9bb672d to 00d57db Compare May 27, 2023 13:01
@Danielsio
Copy link
Collaborator

Keep up the good work, is there screenshots that we can see in the meantime ?

@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 2 times, most recently from b587743 to e2fc76c Compare May 28, 2023 18:35
@MaayanMashhadi
Copy link
Collaborator Author

This is the chat:
chat

this is the page:
the page

@MaayanMashhadi
Copy link
Collaborator Author

Keep up the good work, is there screenshots that we can see in the meantime ?

did it

@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 3 times, most recently from fadccbe to e14b15a Compare May 29, 2023 14:55
@MaayanMashhadi MaayanMashhadi marked this pull request as ready for review May 29, 2023 14:59
@ZvikaNaorCohen
Copy link
Collaborator

Great job, the chat looks awesome! I really like it.

ZvikaNaorCohen
ZvikaNaorCohen reviewed May 29, 2023
View reviewed changes
SchSeba
SchSeba reviewed May 30, 2023
View reviewed changes
Copy link

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

looks good just a small comment

btw if you have time to implement and not part of this PR
you can improve the chat a bit having a 5 sec loop to request the messages (no need to implement a full web socket chat) that will ask the server for messages and reload just the chat box with new messages so you don't need to refresh the page everytime

@MaayanMashhadi
Copy link
Collaborator Author

looks good just a small comment

btw if you have time to implement and not part of this PR you can improve the chat a bit having a 5 sec loop to request the messages (no need to implement a full web socket chat) that will ask the server for messages and reload just the chat box with new messages so you don't need to refresh the page everytime

5 sec loop to request the messages (no need to implement a full web socket chat) that will ask the server for messages and reload just the chat box with new messages so you don't need to refresh the page everytime

Thank you, I open issue for it #103

@amitCohen2
Copy link
Collaborator

Great job!
The tests and all looks great for me! Didn't find any comments to write about it.

Yuval-Vino
Yuval-Vino reviewed May 31, 2023
View reviewed changes
yftacherzog
yftacherzog requested changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@yftacherzog yftacherzog left a comment

Choose a reason for hiding this comment

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

As far as I understand it, this PR includes quite a lot of refactoring that are not directly related to the functionality being introduced. Unless the refactoring is tightly coupled with the code being introduced, it's better to split them into different PRs. It makes it less likely that you'd have uncaught regressions and it also makes it easier to review which makes it less likely to introduce new bugs.

@MaayanMashhadi
Copy link
Collaborator Author

As far as I understand it, this PR includes quite a lot of refactoring that are not directly related to the functionality being introduced. Unless the refactoring is tightly coupled with the code being introduced, it's better to split them into different PRs. It makes it less likely that you'd have uncaught regressions and it also makes it easier to review which makes it less likely to introduce new bugs.

I would like to know if you talked about just the changes of the fixtures I made or other things you think it needs to be in another PR? I think the other changes related to the chat features.

@yftacherzog
Copy link
Contributor

As far as I understand it, this PR includes quite a lot of refactoring that are not directly related to the functionality being introduced. Unless the refactoring is tightly coupled with the code being introduced, it's better to split them into different PRs. It makes it less likely that you'd have uncaught regressions and it also makes it easier to review which makes it less likely to introduce new bugs.

I would like to know if you talked about just the changes of the fixtures I made or other things you think it needs to be in another PR? I think the other changes related to the chat features.

I don't recall anymore. I'll let you be the judge.

@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 3 times, most recently from f8c04c1 to 56d83ec Compare June 5, 2023 06:58
yftacherzog
yftacherzog reviewed Jun 5, 2023
View reviewed changes
yftacherzog
yftacherzog requested changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@yftacherzog yftacherzog left a comment

Choose a reason for hiding this comment

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

Whenever we have an opportunity to test a unit (usually a function) on its own, we should prefer that over testing it inside something else.

We should also have that something-else tested, but then we should focus on its functionality rather than the functionality provided by the unit it utilize.

@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 4 times, most recently from 71059b2 to 193ba76 Compare June 5, 2023 11:23
yftacherzog
yftacherzog reviewed Jun 5, 2023
View reviewed changes
yftacherzog
yftacherzog reviewed Jun 5, 2023
View reviewed changes
yftacherzog
yftacherzog reviewed Jun 5, 2023
View reviewed changes
@MaayanMashhadi MaayanMashhadi force-pushed the chat_branch branch 2 times, most recently from 3ca6ef1 to ac09499 Compare June 5, 2023 16:37
yftacherzog
yftacherzog reviewed Jun 5, 2023
View reviewed changes
@MaayanMashhadi
Add chat to game-event page
7a5c51a 
modify game-event html for adding the chat html
adding tests for the chat
adding new view function that handle the chat
adding urls for the new view function
yftacherzog
yftacherzog approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@yftacherzog yftacherzog left a comment

Choose a reason for hiding this comment

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

lgtm. Good job!

@Yuval-Vino Yuval-Vino merged commit fe181b6 into redhat-beyond:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SchSeba SchSeba SchSeba left review comments

@ZvikaNaorCohen ZvikaNaorCohen ZvikaNaorCohen left review comments

@yftacherzog yftacherzog yftacherzog approved these changes

@Yuval-Vino Yuval-Vino Yuval-Vino approved these changes

@Danielsio Danielsio Danielsio approved these changes

@IdanZimi IdanZimi Awaiting requested review from IdanZimi

@amitCohen2 amitCohen2 Awaiting requested review from amitCohen2

@amitkaplansky amitkaplansky Awaiting requested review from amitkaplansky

@OhadSeg OhadSeg Awaiting requested review from OhadSeg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add chat to game-event page
7 participants
@MaayanMashhadi @Danielsio @ZvikaNaorCohen @amitCohen2 @yftacherzog @SchSeba @Yuval-Vino