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

Project Survey - Andrea Hedström #458

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

Conversation

AndreaHedstrom
Copy link

No description provided.

Copy link

@beemailley beemailley left a 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>
);
}
};

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;

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>

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} />

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>

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.

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