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

Maze #13

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Maze #13

wants to merge 12 commits into from

Conversation

johannacatalinismith
Copy link

Copy link

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Good job with this project Johanna, I like the simplicity which makes it easy to follow. Kudos for the buttons' positions. What about the descriptions though? How should the user know where to go? 👀 Something to think about...

src/App.css Outdated

Choose a reason for hiding this comment

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

Remove unused files and code

src/App.jsx Outdated
Comment on lines 14 to 18
useEffect(() => {
// start("Maze Man");
}, []);

console.log(gameState);

Choose a reason for hiding this comment

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

Remove

src/Game.css Outdated
Comment on lines 1 to 4
.Game {
}

.Game-description {

Choose a reason for hiding this comment

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

Remove unused code throughout your whole app. And make sure to always name CSS class names in kebab-case

Choose a reason for hiding this comment

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

(lowercase)

<button
// ${action.direction} is a template literal, it will be replaced with the value of action.direction
className={`Game-navigate ${action.direction}`}
disabled={loading}

Choose a reason for hiding this comment

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

Very nice! As you know, one of the requirements this week is:

  • Focus on making the UX of your app good. Handle the response delay

and I think it would be very nice with some kind of loading indicator, but on the other hand you have actually handled the response delay by disabling the buttons, so 👍

src/store.js Outdated
username: "",
// here we are declaring the loading after pressing button
loading: false,
gameState: undefined,

Choose a reason for hiding this comment

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

I'd probably use null here

Choose a reason for hiding this comment

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

Your store has the things that is needed and is structured properly 👍

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.

2 participants