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

A Redux labyrinth #193

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

SofiaGerdmar
Copy link

@SofiaGerdmar SofiaGerdmar commented Apr 28, 2023

Copy link

@camcron camcron left a comment

Choose a reason for hiding this comment

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

Really nice job Sofia (and especially in such short amount of time)! I really like the vibe of design you chose and if think it's really suitable for the content of text from the API. You made it feel like an adventure!

One thing to look into is to add a unique identifier for the nameInput that starts the game since it's a shared API. Sometimes you're a sent directly to the last coordinate of the game directly. I think that either a timestamp or a UUID-package could work.

But overall an amazing job!

const initialState = {
user: '',
isStarted: false,
description: '',
Copy link

Choose a reason for hiding this comment

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

Since your changing the state of loading and and it's in the reducer i think it would be good to define it in the initialstate

Comment on lines +65 to +69
fetch('https://labyrinth.technigo.io/action', move)
.then((res) => res.json())
.then((data) => {
dispatch(game.actions.setDescription(data))
})
Copy link

Choose a reason for hiding this comment

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

Look into how you are passing move here if the state is really being updated since it doesn't dispatch anywhere

Comment on lines +36 to +58
const DirectionSection = styled.div`
display: flex;
flex-direction: column;
align-items: center;
`

const MoveBtn = styled.button`
font-family: 'JetBrains Mono', monospace;
background-color: #000;
border-radius: 5px;
padding: 5px 10px;
font-size: 1rem;
cursor: pointer;
margin-bottom: 20px;
color: #F8E152;
border: 2px dotted #F8E152;

&:hover {
background-color: #F8E152;
color: #000;
}
@media (min-width: 668px) {
font-size: 1.2rem;
Copy link

Choose a reason for hiding this comment

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

Take a look at the placement of the styling and if maybe it should be moved outside the useEffect to make the code a bit easier to follow

Comment on lines +20 to +26
.main-container {
display: flex;
flex-direction: column;
align-items: center;
margin-bottom: 30px;
}
.spinner {
Copy link

Choose a reason for hiding this comment

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

I'm not sure about this, but I would look in to best practices and how it's considered to use different kinds of styling in the same project. Maybe it's okay when trying to set a layout!

Comment on lines +9 to +27
useEffect(() => {
const handleKeyDown = (event) => {
switch (event.keyCode) {
case 37: // left arrow
dispatch(nextMove('west'));
break;
case 38: // up arrow
dispatch(nextMove('north'));
break;
case 39: // right arrow
dispatch(nextMove('east'));
break;
case 40: // down arrow
dispatch(nextMove('south'));
break;
default:
break;
}
};
Copy link

Choose a reason for hiding this comment

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

Really nicely done with this switch statement an the use of the arrow keys!

Comment on lines +7 to +19
const StyledForm = styled.form`
display: flex;
flex-direction: column;
align-items: center;
`
const StyledInput = styled.input`
font-family: 'Ubuntu Mono', monospace;
background-color: #FEF5BD;
border: none;
padding: 7px 5px 7px 10px;
outline: none;
font-size: 1rem;
color: #000;
Copy link

Choose a reason for hiding this comment

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

Really like the styling of the whole game and the old school adventure game-feel of it

Comment on lines +18 to +23
{loading && <Loading />}
{!loading && (
<div>
{!gameStarted && <StartGame />}
{gameStarted && <Game />}
</div>
Copy link

Choose a reason for hiding this comment

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

Super clean code!

flex-direction: column;
align-items: center;
height: 100vh;
width: 100vw;
Copy link

Choose a reason for hiding this comment

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

Great and responsive styling. Just take a look at mobile and width and what it might be that's causing a side scroll from 375px and smaller

Copy link
Author

Choose a reason for hiding this comment

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

Could you take a screen-shot of the side scroll so I can see when it appears? I can't seem to recreate it in inspector.

margin-bottom: 10px;
}
.direction-div p {
text-align: justify;
Copy link

Choose a reason for hiding this comment

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

Really liked that you used justify instead of center here!

<div key={action.direction} className="direction-div">
<p>{action.description}</p>
<div>
<MoveBtn type="button" onClick={() => moveOnClick(action.direction)}>Go {action.direction}</MoveBtn>
Copy link

Choose a reason for hiding this comment

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

Nicely done with the stretch goal of showing the direction in it's button

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