Joyce Kuo: Project Business Site #1
Conversation
There was a problem hiding this comment.
Good job on your business site project Joyce! 🎸 Some feedback and things to think about:
HTML
- Your HTML looks well structured! Just remember to keep all layouting to the CSS file, i.e. instead of using the out-of-date
brtag, you could for example use flexbox - The form is well structured but I saw an input with a missing id attribute (and another input that didn't need it because the label was wrapping the input already). Please update this
CSS
- Good idea with a global reset to ensure consistency!
- Nice "hero" section with the quotes! It's wrapping nicely 🎁
Overall really good job with the project, but some changes are needed 👇
Changes Requested
- Update the form action so that we can test your form
- Make sure all labels and inputs are connected properly with for / id attributes
code/index.html
Outdated
|
|
||
| <div class="form-container" id="review"> | ||
| <div class="form"> | ||
| <form action="https:httpbin.org/anything" method="post"> |
There was a problem hiding this comment.
This link is faulty, it should be: https://httpbin.org/anything
There was a problem hiding this comment.
OK, link should be correct now 🔗
code/style.css
Outdated
| font-family: 'Cinzel', sans-serif; | ||
| font-size: clamp(12px, 2vw, 20px); | ||
| font-weight: bold; | ||
| transition: color 0.3s, background-color o.3s; |
|
I removed the br tags in the html form and adjusted the css file instead, and I added the missing attribute/removed the unnecessary attribute in the form as well. Thank you very much for your feedback ! 😊 |
There was a problem hiding this comment.
Great work on your business site, Joyce! The responsiveness works great on all sizes and I really like the animation of the logo 🤩 The form works just as expected as well 👏
For readability one suggestion would be to set a maximum width to containers of text blocks (like the one under the hero), so that a line of text is about 9-11 words per line (on desktop anyway).
Hope my comments make sense. Otherwise let me know.
Keep up the good work ⭐
| <body> | ||
| <header class="header"> | ||
| <div class="top-row"> | ||
| <div class="hamburger"> |
There was a problem hiding this comment.
I know it's not a functioning hamburger menu right now, but in the future, remember that for the hamburger menu to be accessible for keyboard users and people using screen readers it needs to be a <a> or <button>.
| </ul> | ||
| <div class="social-media"> | ||
| <a href="https://www.facebook.com/theroyalswedes" target="_blank"> | ||
| <img src="./images/fb.png"> |
There was a problem hiding this comment.
All <img> tags should have alt="" with a descriptive text. This shows up if the image is for some reason broken and is also read by screen readers to explain the image to the user. Can also be good for SEO in some cases.
I'll just comment that on this image element, but it goes for all of them.
| Whether | ||
| it's | ||
| reliving the glory days of rock or creating new memories on the dance floor, The Royal Swedes know how to turn | ||
| any event into a celebration fit for royalty.</p><br> |
There was a problem hiding this comment.
Instead of using <br> perhaps you could work with multiple <p> to divide each sentence into different paragraphs.
| <source src="./video/royal-swedes-live - Made with Clipchamp.mp4" type="video/mp4"> | ||
| </video> | ||
| <div class="grid-item"> | ||
| <h2>"One of the best groups I have seen in a long time. Their music has a wide variety, they cater for all tastes, |
There was a problem hiding this comment.
For semantic purposes perhaps you could use <blockquote> for the quotes and <cite> for the name of the person who said the quote. Read more here »
Then if you wanted a specific styling for all quotes you could apply it to all blockquotes and not have to add a specific class for it for example.
| </div> | ||
|
|
||
| <div class="form-container" id="review"> | ||
| <div class="form"> |
There was a problem hiding this comment.
Not sure you need this <div>, you could probably just add the class .form to the actual form element. Or apply the styling you have for the class .form to the form element directly.
| .nav-menu li a:hover { | ||
| color: purple; | ||
| background-color: #f1f1f1; | ||
| ; |
There was a problem hiding this comment.
Small thing, but this extra ; is not needed.
| justify-content: space-between; | ||
| width: 100%; | ||
| padding: 0 20px; | ||
| flex-grow: 1; |
There was a problem hiding this comment.
I'm not sure this is doing anything? Considering you already have width: 100%; and justify-content: space-between;, I think it might not be needed.
| text-decoration: none; | ||
| color: black; | ||
| padding: 8px 16px; | ||
| font-family: 'Cinzel', sans-serif; |
There was a problem hiding this comment.
Looking at Cinzel it is a serif font, so maybe you would like the fallback to also be a serif and not a sans-serif?
| box-shadow: 0 0 10px rgba(0, 0, 0, 0.1); | ||
| box-sizing: border-box; | ||
| font-family: Arial, Helvetica, sans-serif; | ||
| font-size: clamp(14px, 2vw, 18px); |
There was a problem hiding this comment.
Cool function for setting min and max font-size! Didn't know about clamp 👏
| padding: 0; | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| body { |
There was a problem hiding this comment.
Since you are using Cinzel as font in most places consider adding it as a font-family on the body instead of declaring it on multiple elements. Then you can declare the other font, Oswald, on the specific places you want it. On all headings for instance.
A business site for a cover band:
https://theroyalswedes.netlify.app/