-
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
Project Survey - Andrea Hedström #458
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.
Overall, everything looks good! Your code looks very clean and very easy to read. The components are easy to understand. Good job!
</div> | ||
); | ||
} | ||
}; |
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 like that your App.js is very tidy and organized. It's very easy to see how everything is related.
margin-left: 25px; | ||
margin-right: 25px; | ||
border-radius: 10px; | ||
|
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.
So, I won't comment a ton on your CSS - you've already said you're updating it. But, I wanted to share "vw" and "vh" as measurements for view width and view height. If you've already experimented with these, ignore me. They were new to me as I was styling this project and were SUPER helpful in forcing my survey to take up the amount of space that I wanted it to.
id="drink" | ||
value={drink} | ||
onChange={handleDrinkChange}> | ||
<option value="">Menu</option> |
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 think you can add "disable" either before or after "value" for the MENU line and that will stop being an acceptable value. Then the user would have to pick something else from the dropdown.
className="radioBtn" | ||
value={group} | ||
onChange={handleFoodChange} | ||
checked={food === group} /> |
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 think an easy way to add a bit of accessibility is to add an aria-label here. Something like:
aria-label={group}
Then the label should change for each radio button.
})) | ||
}}>MAGIC PARTYBUTTON | ||
</button> | ||
|
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.
The party button is such a cute addition! And I like that the confetti colors match your design scheme.
No description provided.