search

nmoskaleva / hyf-homework

Fork this repository to get your template for handing in homework
0 stars 0 forks source link

Feedback on homework week #1 #3

Open AThraen opened 5 years ago

AThraen commented 5 years ago

https://github.com/nmoskaleva/hyf-homework/blob/498a239bad4c7f4e5c860ef267eaf28e20e9e653/html-css/week1/index.html#L7

Privet Natalia. Here is some more in-depth review of your homework for week 1. Sorry for the delay. As I wrote on slack, overall it looks really good (очень хорошо!). There are a few minor details though: 1) The line referenced in this issue above (line 7) a link to a few google fonts. That's all fine, but technically the line breaks the official W3C validator for valid html syntax because the pipe '|' character is not allowed in a url. The solution is easy though - just replace it with the URL-encoded value for the same character: '%7C' (I bet you have seen the % notation often in urls in your browser - that's url-encoding). 2) As I also wrote on Slack. It's great that you are using Header, Footer (although that is empty?) and Aside - but it would be great to see more semantic tags like Main, Section and so on. W3 Schools has a good list of the most commonly used here: https://www.w3schools.com/html/html5_semantic_elements.asp 3) Your picture is fairly large 3.4 MB and takes a while to load on the site. It could easily be resized and optimized without losing quality to at least 1/10th of the current file size. Also you have resized it with in-line styling. It would make more sense if you gathered all your styling in the style sheet. 4) It is not built adaptive or responsive which means that it's not ideal to read on for instance mobile devices (where it all just becomes small). As far as I know that wasn't part of the scope for the assignment (as the foundation for this was first mentioned in class last sunday), so this is just a hint that trying to make a flexible layout can improve the mobile experience a lot. I can see that you tried to start on this in the CSS with the media query, but it doesn't seem to be working as expected in my browser. 5) Some of your indentation in the html is a bit mixed up, which makes it harder to read - it's only a few places though - like around the <ul> tags.

I think that's all I have. Keep up the good work!

nmoskaleva commented 5 years ago

Hi Allan! Thanks a lot for the feedback, it helped so much! I followed the recommendations and also tried to make it responsive with flexbox.