Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Taylor task11 #14

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Taylor task11 #14

merged 4 commits into from
Nov 7, 2022

Conversation

taylor-stevens
Copy link
Collaborator

@taylor-stevens taylor-stevens commented Nov 6, 2022 •
edited
Loading

Create the line of communication from the backend to the frontend used to populate the second section of the UI’s associated unfriend button.

  • Create the React hook for friendsChange
  • Add a friends list parameter to TownController
  • Add a friends changed event to TownEvents in TownController

@taylor-stevens taylor-stevens added documentation Improvements or additions to documentation enhancement New feature or request tests New tests for the added code labels Nov 6, 2022
@taylor-stevens taylor-stevens added this to the Sprint 2 milestone Nov 6, 2022
@taylor-stevens taylor-stevens self-assigned this Nov 6, 2022
@taylor-stevens taylor-stevens changed the base branch from main to sprint1.5/2-dev November 6, 2022 18:52
dylanmarin
dylanmarin approved these changes Nov 6, 2022
View reviewed changes
Copy link
Contributor

@dylanmarin dylanmarin left a comment

Choose a reason for hiding this comment

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

lgtm but there are just a couple lint checks from testing. I think chloes PR may add the tests back that use the 'unused' vars so maybe you can ignore

frontend/src/classes/TownController.ts Outdated Show resolved Hide resolved
@dylanmarin
Copy link
Contributor

test look good to me

chloe684
chloe684 requested changes Nov 6, 2022
View reviewed changes
Copy link
Contributor

@chloe684 chloe684 left a comment

Choose a reason for hiding this comment

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

Nice job! Just a couple nitpicky comments to address

chloe684
chloe684 approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@chloe684 chloe684 left a comment

Choose a reason for hiding this comment

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

Fixed the suggested stuff - looks good

jafedor
jafedor reviewed Nov 7, 2022
View reviewed changes
Copy link
Contributor

@jafedor jafedor left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. My only other thought is to maybe add a comment at the top of the beforeEach blocks in the test files saying what they're setting up, since it's not immediately obvious from a quick glance the data each test is starting with.

frontend/src/classes/TownController.ts Outdated Show resolved Hide resolved
@taylor-stevens taylor-stevens merged commit 874f5de into sprint1.5/2-dev Nov 7, 2022
@taylor-stevens taylor-stevens deleted the taylor_task11 branch November 7, 2022 13:19
jafedor
jafedor reviewed Nov 7, 2022
View reviewed changes
Copy link
Contributor

@jafedor jafedor left a comment

Choose a reason for hiding this comment

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

Final approval after suggested changes were made.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jafedor jafedor jafedor left review comments

@dylanmarin dylanmarin dylanmarin approved these changes

@chloe684 chloe684 chloe684 approved these changes

Assignees

@taylor-stevens taylor-stevens

Labels
documentation Improvements or additions to documentation enhancement New feature or request tests New tests for the added code
Projects
None yet
Milestone
Sprint 2
Development

Successfully merging this pull request may close these issues.
4 participants
@taylor-stevens @dylanmarin @chloe684 @jafedor