Jason.Holloway User-Interface-Project-Week #574
Conversation
… mobile media query
…u header and opacity
There was a problem hiding this comment.
Functional Components - II
MVP
- Build layout HTML/LESS: Home page desktop design
- Build layout HTML/LESS: Home page mobile design
- Build layout HTML/LESS: Services page desktop design only
- Build layout HTML/LESS/JavaScript: Navigation system design (expanded and non)
- Build Custom Component HTML/LESS/JavaScript: Services page tab navigator design
What Looks Great
- Great work matching the design files
- Frequent and descriptive commits
- Code easy to read
What Could Be Improved
- Code needs better formatting - you should only use id in case of emergency. Overuse could mean that you have a weak understanding of how to style.
- Descriptive class names
- Add more comments so code is easier for other programmers as well as yourself
What Could Be Added
- More Comments esp in the js so anyone can tell what each chunk of code should do
- Separate js pages for each component.
Rating (1-3)
2
| @@ -11,8 +13,84 @@ | |||
| </head> | |||
|
|
|||
| <body> | |||
| <div class="container12"> | |||
There was a problem hiding this comment.
I can see how container number might initially work, but what happens when you get to container100 and you can't remember what each of them control. Container should be reserved for the main wrapper only and the other receive descriptive names eg: container19 = menu_container
|
|
||
| </div> | ||
|
|
||
| <div class="container21"> |
| </ul> | ||
| </div> | ||
| </header> | ||
| <img id="pic1" src="img\home\home-jumbotron.png" alt="jumbo" style="width:100%"> |
There was a problem hiding this comment.
neither of these should have a width. I think I told you about it twice but maybe you missed looking inline
| <div class="container2"> | ||
|
|
||
| <div class ="picwrapper"> | ||
| <img id="pic1" class="notepad" src="img\home\home-img-1.png" alt="penandpad"> |
There was a problem hiding this comment.
Why did these need an id? nothing on this project should require that specific weight
|
|
||
| Enter email | ||
|
|
||
| </civ> |
There was a problem hiding this comment.
? This is what clean code helps prevent. This could have caused bugs.
|
|
||
| <h6>copyright © 2018 Smith and Jones </h6> | ||
|
|
||
| </div> |
There was a problem hiding this comment.
This doesn't need two containers. class copyright would have been sufficient.
| } | ||
|
|
||
| // SERVICES TABS |
|
|
There was a problem hiding this comment.
I would advise for future projects to use a separate js file for each feature. Just for readability, reusability and ease of debugging
|
|
||
| <h1>Services</h1> | ||
| </div> |
|
|
||
| </div> | ||
|
|
||
| <div class="container16"> |
There was a problem hiding this comment.
don't forget to reuse components like nav and footer that match across pages. this saves time and creates cleaner code as well as pages that match.
@VivaCode