-
Notifications
You must be signed in to change notification settings - Fork 471
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
Concert House #364
base: master
Are you sure you want to change the base?
Concert House #364
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.
Good job with this business site Gitte! As I mentioned on your demo, I really like the yellow on the black/white picture 🥳 Nice and accessible contrasts.
HTML
- The header includes a logo and text elements wrapped in a div with the class hero, which is a nice, organized approach. However, instead of using a custom
<logo>
element, it's better to stick to standard HTML tags like<div>
or<header>
, as<logo>
is not a recognized HTML tag. - For the links inside the navigation, make sure you include text between the tags for better accessibility and user interaction. Right now, they seem to be empty. And no need for the
<ul>
element.
CSS
- I'm not a designer but I've learnt a thing or two, so one tip I would like to give you is to align all your elements (labels have a left padding and inputs don't)
- Adding some padding around the placeholders in the input would look nice
- I think the yellow color you chose for the hero would be a nice contrasting color to play around with in the form as well. Maybe as a background color on the button and potentially as a border or outline/focus too?
Overall, you're on the right track, and your project is nicely executed! Keep up the great work! 🎉
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.
-
Clear Structure: Nice use of semantic tags like header, footer, and proper form elements like label and input. It makes the code easier to follow and better for search engines.
-
Mobile Friendly: The media queries you've added do a great job making sure the form and header look good on different screen sizes.
-
User-Friendly: Adding the required attributes to the form fields is a great touch—it ensures users fill in all the important info without missing anything.
|
||
<!-- video or image as a header is cool :) --> | ||
<head> |
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.
There are duplicated head and body tags, which can cause rendering issues.
/* footer for mobile phone */ | ||
footer { | ||
display: flex; | ||
display: row; |
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.
display: row; isn't a valid value. Correct way to set row layout (default for flex): flex-direction: row;
code/index.html
Outdated
<div class="hamburger-layer"></div> | ||
<nav> | ||
<ul> | ||
<a class="a-header" href="http://google.com"></a> |
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 elements within the navigation don’t have visible text. This might confuse users and affect accessibility.
https://concert-house.netlify.app/