-
Notifications
You must be signed in to change notification settings - Fork 435
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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.
https://wondrous-gelato-35a604.netlify.app
In progress