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

Tic Tac Toe Game #208

Merged
merged 12 commits into from
May 24, 2022
Merged

Tic Tac Toe Game #208

merged 12 commits into from
May 24, 2022

Conversation

tejinder-sharma
Copy link
Member

@tejinder-sharma tejinder-sharma commented May 20, 2022

First thing, PLEASE READ THIS: ReactPlay Code Review Checklist

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@vercel
Copy link

vercel bot commented May 20, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @atapas on Vercel.

@atapas first needs to authorize it.

@tejinder-sharma
Copy link
Member Author

Please see and review the Game.

@atapas atapas self-requested a review May 20, 2022 07:40
@vercel
Copy link

vercel bot commented May 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-play ✅ Ready (Inspect) Visit Preview May 24, 2022 at 2:02AM (UTC)

Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

Thanks @tejinder-sharma for putting this play!

I have posted some review comments. Please check and let us know when the changes are done.

Here is the check list you can go through as well: https://github.com/atapas/react-play/wiki/ReactPlay-Code-Review-Checklist

Additionally, Please update the description of the PR.

Thanks!

src/meta/play-meta.js Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/TicTacToeGame.jsx Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/TicTacToeGame.jsx Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/TicTacToeGame.jsx Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/TicTacToeGame.jsx Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/TicTacToeGame.jsx Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/tic.css Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/tic.css Outdated Show resolved Hide resolved
src/plays/tic-tac-toe-game/tic.css Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align the text in header using flex-box:

display: flex;
align-items: center;
justify-content: center;

@tejinder-sharma
Copy link
Member Author

Thank You for reviewing the header too.

@tejinder-sharma
Copy link
Member Author

Hello Tapas and team,

I have made the appropriate changes but I am finding difficulty i.e

  1. When a player won corresponding three squares are not getting the same color using functional components.
  2. The console is showing a Component Unmount warning, I will work on that.

It is very helpful if you suggest what we can do on the first point.

@Programming-School-Pro-Coding
Copy link
Contributor

Programming-School-Pro-Coding commented May 22, 2022

Hello Tapas and team,

I have made the appropriate changes but I am finding difficulty i.e

  1. When a player won corresponding three squares are not getting the same color using functional components.
  2. The console is showing a Component Unmount warning, I will work on that.

It is very helpful if you suggest what we can do on the first point.

Ok, Very Amazing Work

@tejinder-sharma
Copy link
Member Author

Hello Team,

I have resolved the above two issues. Now the game is perfectly working with no errors/warnings.

Please review the code.

@Programming-School-Pro-Coding
Copy link
Contributor

Hello Team,

I have resolved the above two issues. Now the game is perfectly working with no errors/warnings.

Please review the code.

Please resolve conflicts

@atapas
Copy link
Member

atapas commented May 23, 2022

Hello Team,

I have resolved the above two issues. Now the game is perfectly working with no errors/warnings.

Please review the code.

@tejinder-sharma Could you please mark the comments as resolved(for the ones the review comments have been taken care). We will start a review once you resolved the comments as in it will help to identify what's taken care of and what's pending.

Also, kindly resolve the merge conflicts:

image

@tejinder-sharma
Copy link
Member Author

Tapas and Team I have resolved the conflicts.

Copy link
Member Author

@tejinder-sharma tejinder-sharma left a comment

Choose a reason for hiding this comment

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

I have resolved the mentioned issues.

src/plays/tic-tac-toe-game/TicTacToeGame.jsx Show resolved Hide resolved
src/plays/tic-tac-toe-game/styles/tic.css Outdated Show resolved Hide resolved
src/meta/play-meta.js Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

LGTM

@atapas
Copy link
Member

atapas commented May 24, 2022

Hi @tejinder-sharma are you still making code changes?

@tejinder-sharma
Copy link
Member Author

Now everything is completed from my side. just one thing I forgot that was destructuring now it is completed.

Now, No more changes from my side.

@atapas atapas merged commit 7c0fb2d into reactplay:main May 24, 2022
@atapas
Copy link
Member

atapas commented May 24, 2022

@all-contributors please add @tejinder-sharma for Code

@allcontributors
Copy link
Contributor

@atapas

I've put up a pull request to add @tejinder-sharma! 🎉

@atapas
Copy link
Member

atapas commented May 24, 2022

@tejinder-sharma Thanks for your contribution. Please do not stop with one. Keep contributing and make the project and community stronger. Thanks!

Don't forget to let the world know about your contributions!!!

@tejinder-sharma
Copy link
Member Author

Wow, Thank you Tapas and Team, Your support from the start is like a teacher who told his student what to improve or what not to be. I have learned a lot. With this confidence, I will contribute more and more.🙌

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.

3 participants