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

feat: add tictactoe example by hackyguru #292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented Dec 8, 2023

@weboko @danisharora099 Can you check? I am not sure if I missed anything necessary to get this in and deployed!

@vpavlin vpavlin requested a review from a team as a code owner December 8, 2023 10:29
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

hm. not a fan of using tailwindcss extensively but also relying on native CSS heavily at the same time

Comment on lines +98 to +99
let b = {};
let o = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: some let keywords can be swapped for const keywords in the codebase

import ShortUniqueId from 'short-unique-id';
import Router from 'next/router';

export default function Hero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the relevance of this component name

if (room === null) {
setRoom(uid.rnd());
}
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add room as a dependency to the useEffect

Comment on lines +25 to +26
sessionStorage.setItem('roomId', room);
sessionStorage.setItem('player', 'x');
Copy link
Contributor

Choose a reason for hiding this comment

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

would be cool to use enums for this

};

const handleJoinGameClick = () => {
setGame('join');
Copy link
Contributor

Choose a reason for hiding this comment

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

similar comment as above re enums

Copy link
Contributor

Choose a reason for hiding this comment

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

this file can be broken down into more files for more readability

@weboko
Copy link
Contributor

weboko commented Jan 23, 2024

@vpavlin currently there is a problem with hosting Next.js apps - still didn't figure solution

@danisharora099
Copy link
Contributor

@vpavlin currently there is a problem with hosting Next.js apps - still didn't figure solution

Can you elaborate the problem (in an issue if necessary)?

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

Successfully merging this pull request may close these issues.

3 participants