Skip to content
This repository was archived by the owner on Apr 28, 2026. It is now read-only.

feat: add tictactoe example by hackyguru - #292

Open
vpavlin wants to merge 1 commit into
masterfrom
feat/tictactoe
Open

vpavlin wants to merge 1 commit into
masterfrom
feat/tictactoe

Conversation

@vpavlin

@vpavlin vpavlin commented Dec 8, 2023

Copy link
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

weboko commented Jan 23, 2024

Copy link
Copy Markdown
Contributor

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

@danisharora099

Copy link
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants