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

Welcome to the Adventure! Daniel & Carol #211

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

Conversation

dannebrob
Copy link

Copy link

@theresBL theresBL left a comment

Choose a reason for hiding this comment

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

Overall such a great job with this project! The code looks very clean and it is easy to follow. The components is easily named so you get an understanding to what they are supposed to do.

The one thing that could make it even more clear would be to pick one styling option to go with thorough out the project.

As said you have done a great job and we are impressed with how clean the code looks. Bee proud!

Comment on lines +64 to +71
<Container>
{loading ? (
<Loading />
) : username === '' ? (
<LandingPage />
) : (
<GameBoard />
)}
Copy link

Choose a reason for hiding this comment

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

Nice set up!

Comment on lines +1 to +12
// src/backgroundImages.js
const backgroundImages = {
'0,0': '/assets/img2.jpg',
'1,0': '/assets/img3.jpg',
'1,1': 'assets/img4.jpg',
'0,1': 'assets/img5.jpg',
'0,2': 'assets/img6.jpg',
'0,3': 'assets/img7.jpg',
'1,3': 'assets/img8.jpg'
};

export default backgroundImages;
Copy link

Choose a reason for hiding this comment

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

Clean and nicely done to keep this in its own component!

window.location.reload();
};

const currentBackground = game && backgroundImages[coordinates];
Copy link

Choose a reason for hiding this comment

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

Nice solution with the different backgrounds! 👍🏼

}

Copy link

Choose a reason for hiding this comment

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

Over all great work with the styling and you have mostly used styled components with you have done excellently. Could you to next project try to make the styling uniformly?

Comment on lines +14 to +26
.animation {
-webkit-animation: slidein 25s;
animation: slidein 25s;

-webkit-animation-fill-mode: forwards;
animation-fill-mode: forwards;

-webkit-animation-iteration-count: infinite;
animation-iteration-count: infinite;

-webkit-animation-direction: side;
animation-direction: side ;
}
Copy link

Choose a reason for hiding this comment

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

We actually can't see this animation witch is sad. It would be cool to see how this would look like?

Comment on lines +8 to +15
const Background = styled.div`
background-image: url('/assets/img1.jpg');
background-size: cover;
background-position: center;
height: 100vh;
margin: 0;
padding: 0;
`;
Copy link

Choose a reason for hiding this comment

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

Great job using styled components 👏🏻

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