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

Oscar's Project Survey #448

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

Conversation

OscarSindemark
Copy link

Copy link

@AnnikaSonnek AnnikaSonnek left a comment

Choose a reason for hiding this comment

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

I ran your website through the W3 Validator and there were no issues there. Yay!

I also ran it through the WAVE accessibility tool and there where some contrast issues with the green text of the step counter, no issues with the header though. I think its because the header is bold. There were also some form elements missing around the select and text-input. I saw that you made the big container surveyForm into a form but maybe it would be good to have that one as a div instead and then add the form element in each of the components instead.

I also checked the responsiveness and it's not responsive for mobile view.

It would be nice with some comments maybe but the code is easy to follow without them as well. Good job!


{step < 5 && (
<>
<p>Current page: {step} </p>

Choose a reason for hiding this comment

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

It was a nice feature that you the user could see which step they were on.

Choose a reason for hiding this comment

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

I LOVE ATTACK ON TITAN! I was so happy to see it among the options! Hahah!

<input type="radio" value="Love Actually" />
Love Actually
</div>
<div className="eeaao">

Choose a reason for hiding this comment

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

Love the name of this div! Hahah! Such a long movie title!

@@ -5,9 +5,23 @@ body {
sans-serif;
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
background-image: url('./components/Assets/movies-background.jpg');

Choose a reason for hiding this comment

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

I liked that you had an image as a background!

Mad Men
</div>
<div className="attackOnTitan">
<img src={require('./Assets/aot.jpg')} alt="The show Attack on Titan" />

Choose a reason for hiding this comment

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

What does "require" mean in this context? I've never seen it before!

Find me in src/app.js!
</div>
);
<form className="surveyForm">

Choose a reason for hiding this comment

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

I see that you made a form instead of a div here, how come?
Also, if you want to style it so that it's not as wide you can specify that with the max-width or width in the CSS.

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