London 10 | Shahid Amin | HTML/ CSS Week 3 - #233
Conversation
SallyMcGrath
left a comment
SallyMcGrath
left a comment
There was a problem hiding this comment.
Thanks so much for your hard work! I can see you've put a lot of effort into this. A perfect 100 score on Lighthouse.
I've made a few small comments and dropped a few questions for you to think about. Take a look and have a think. And try the stretch task! It's a good exercise in refactoring and testing.
| /> | ||
| </section> | ||
|
|
||
| <section> |
There was a problem hiding this comment.
What is section doing for you here that fieldset is not? Take look in the Accessibility API to view your HTML in a different way.
| type="date" | ||
| name="delivery" | ||
| id="delivery" | ||
| min="2023-01-16" |
There was a problem hiding this comment.
Does this meet all the requirements listed in the readme? Take another look
| </form> | ||
| </main> | ||
| <footer> | ||
| <h2>By Shahid Amin</h2> |
| } | ||
|
|
||
| *, | ||
| *:before, |
There was a problem hiding this comment.
| *:before, |
Do you need to set box sizing rules for pseudo content when you haven't defined any pseudo content?
| margin: 3rem 20rem; | ||
| padding: 1.5rem; | ||
| height: 100%; | ||
| line-height: 2rem; |
There was a problem hiding this comment.
| line-height: 2rem; | |
| line-height: 2; |
Never use length units on a line height. It will mess you up in more complex designs.
|
|
||
| main { | ||
| font-size: 20px; | ||
| margin: 3rem 20rem; |
There was a problem hiding this comment.
This margin is pretty large. Would it make sense to set a max-width on this content and margin: auto? Why might I be suggesting this change?
2 participants
No description provided.