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

Linda and Sofies forest path #5

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

Conversation

SofieFerrari
Copy link

Netlify link

https://forest-path-of-deployment-horror.netlify.app/

Collaborators

[linda-f, SofieFerrari]

Copy link

@Martin-Joensson Martin-Joensson left a comment

Choose a reason for hiding this comment

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

Cool site! We enjoyed your maze!

We feel that you meet the requirements, maybe you could guide the user a little bit more. You made the maze harder UX-vise by moving the buttons for example. (Check our comment below =))

The images take a while to load and could probably be smaller without sacrificing quality.

Well done this week! And go Team Moon!
/Martin & Etna

Choose a reason for hiding this comment

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

Very Indiana Jones! =)
/Martin & Etna

};

changeBackgroundImage(coordinates);
}, [coordinates]);

Choose a reason for hiding this comment

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

Nice approach to changing the backgrounds! We did the same thing ;)
/Martin & Etna

}}
>
{action.direction}
</button>

Choose a reason for hiding this comment

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

For future projects you might want to sort the map so that the same direction ends up in the same place. Like north always being on top for example. You could for example use {action.direction} as a class name to target specific buttons with CSS.

You can also call it a feature, not a bug, because it makes the maze a little harder =D

/Martin & Etna

alert("Please set a username. Thx!");
} else {
fetchStart(userName);
}

Choose a reason for hiding this comment

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

Nice that you check for a Username!
/Martin & Etna

className="input-field"
id="user-input"
type="text"
placeholder="chose something rare"

Choose a reason for hiding this comment

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

Small typo =) We think it should be "Choose"
Good to have that instruction for the user. But what is rare?

/Martin & Etna

method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
username: userName,

Choose a reason for hiding this comment

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

Here you could make the username unique in some way to let the user type in what they want. You could add a random number after the username, or the current date as a string, for example.

    const uniqueUsername = username + Date.now();
    method: "POST",
    headers: { "Content-Type": "application/json" },
    body: JSON.stringify({
      username: uniqueUsername,

/Martin & Etna

Choose a reason for hiding this comment

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

Great idea!


if (!response.ok) {
throw new Error(
"Failed to Start your adventure. Please reload the page."

Choose a reason for hiding this comment

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

We like the themed error message. Very cool!

/Martin & Etna

}

.action-button:hover {
background: linear-gradient(to top, #4a6741, #374f2f, #22311d);

Choose a reason for hiding this comment

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

Since it's the same background as for the button you don't need to specify it again for the hover-state.

/Martin & Etna

margin-top: 50px;
height: auto;
width: 350px; ;
}

Choose a reason for hiding this comment

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

Did you want to put the image in the center? In that case you need to use display: flex on the parent div ( .header-image{} ). There is also a stray ; on line 7.

You could probably do this, if you want it centered:

.header-image {
display: flex;
justify-content: center;
align-items: center;
}

.header-image img {
margin-top: 50px;
height: auto;
width: 350px;
}

/Martin & Etna

Copy link

Choose a reason for hiding this comment

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

nope we didn't want it center 🙈

}

.background-picture {
background-image: url("startPage.jpg");

Choose a reason for hiding this comment

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

Picture not showing up when you load the page. Is it a broken path? =S

/Martin & Etna

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 your labyrinth!

<div className="action-box">
<p>{description}</p>

{actions.map((action) => (

Choose a reason for hiding this comment

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

Would be nice if you also shared the descriptions of the different directions to help guide the user what to choose

Choose a reason for hiding this comment

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

Remove the files you're not using

Choose a reason for hiding this comment

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

Formatting

};

return (
<>

Choose a reason for hiding this comment

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

Unnecessary fragment

method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
username: userName,

Choose a reason for hiding this comment

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

Great idea!

Comment on lines +34 to +38
set({ start: data });
set({ gameFlow: true });
set({ actions: data.actions });
set({ description: data.description });
set({ coordinates: data.coordinates });

Choose a reason for hiding this comment

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

You can group these in one set

Comment on lines +47 to +48
set({ loading: true });
set({ gameFlow: true });

Choose a reason for hiding this comment

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

Same with these

Comment on lines +68 to +70
set({ actions: data.actions });
set({ description: data.description });
set({ coordinates: data.coordinates });

Choose a reason for hiding this comment

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

And these

</div>
);
<>
{gameFlow ? <DisplayLabyrinth /> : <UserInput />}

Choose a reason for hiding this comment

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

I don't understand the name of this state to be honest 👀 Is it supposed to be like a hasStartedGame kind of variable?

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.

4 participants