Skip to content

Oscar's Project Survey#448

Open
OscarSindemark wants to merge 4 commits intoTechnigo:masterfrom
OscarSindemark:master
Open

Oscar's Project Survey#448
OscarSindemark wants to merge 4 commits intoTechnigo:masterfrom
OscarSindemark:master

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!

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