html-css/Ali #2
html-css/Ali #2
Conversation
Homework files uploaded
There was a problem hiding this comment.
Well done. Besides learning to use more semantic tags, you are on a good track.
Unfortunately, I cannot approve your PR before you delete the duplicate files as seen in the screenshot. It's highly possible, that someone else might upload the same named file in the main html-css/week1 folder and overwrite yours.
| <a href="#">SUMMARY</a> | ||
| </li> | ||
| <li> | ||
| <a href="#">SKILLS</a> | ||
| </li> | ||
| <li> | ||
| <a href="#">PORTFOLIO</a> | ||
| </li> | ||
| <li> | ||
| <a href="#">CONTACT</a> |
There was a problem hiding this comment.
You've forgotten to add the links that would lead to each section
| <a href="#">SUMMARY</a> | |
| </li> | |
| <li> | |
| <a href="#">SKILLS</a> | |
| </li> | |
| <li> | |
| <a href="#">PORTFOLIO</a> | |
| </li> | |
| <li> | |
| <a href="#">CONTACT</a> | |
| <a href="#summary">SUMMARY</a> | |
| </li> | |
| <li> | |
| <a href="#skills">SKILLS</a> | |
| </li> | |
| <li> | |
| <a href="#portfolio">PORTFOLIO</a> | |
| </li> | |
| <li> | |
| <a href="#contact">CONTACT</a> |
| <div id="name"> | ||
| Ali Nematollahi | ||
| </div> | ||
| <div id="title"> | ||
| Full-Stack Developer | ||
| </div> |
There was a problem hiding this comment.
Div tags don't hold any semantic meaning. They just wrap content. Instead, you could use one of the H1 to H6 headlines.
For example:
| <div id="name"> | |
| Ali Nematollahi | |
| </div> | |
| <div id="title"> | |
| Full-Stack Developer | |
| </div> | |
| <h1 class="header__title">Ali Nematollahi </h1> | |
| <p class="header__description">Full-Stack Developer</p> |
| <h4 class="section-title"> | ||
| SUMMARY | ||
| </h4> |
There was a problem hiding this comment.
It's recommended not to jump the title order. For example, you should first have on the page somewhere an H1, then follow up with h2, h3, and just then use a h4. You cannot jump directly to h4, without breaking some accessibility features. A bit better explanation can be found in the 3rd bullet point here - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#usage_notes
| <div></div> | ||
| <div class="section-item"> HTML and CSS</div> | ||
| <div></div> | ||
| <div class="section-item"> JavaScript </div> | ||
| <div></div> | ||
| <div class="section-item">TypeScript </div> | ||
| <div></div> | ||
| <div class="section-item"> React.js </div> | ||
| <div></div> | ||
| <div class="section-item"> Node.js </div> | ||
| <div></div> | ||
| <div class="section-item"> MySQL and MongoDB </div> | ||
| <div></div> | ||
| <div class="section-item"> Restfull API </div> | ||
| <div></div> | ||
| <div class="section-item"> GraphQL </div> | ||
| <div></div> | ||
| <div class="section-item"> Git and GitHub </div> |
There was a problem hiding this comment.
You could take a few different approaches here.
-
Remove the empty
<div></div>and change the other<div>tags to<span>. Then let them stack as multiples on one line - there is nothing wrong with that. -
Same as above related to removing the empty
<div></div>, then use flex-box on#skills(preferably you would use a class selector instead of ID):
display: flex;
flex-direction: column;
align-items: flex-start;
| <div class="contact-title">Address</div> | ||
| <div class="section-item"> Copenhagen, Denmark</div> | ||
| <div class="contact-title">Mobile</div> | ||
| <div class="section-item">+45 71557785</div> |
There was a problem hiding this comment.
You can find a good option for <a href=""> for making phone numbers easier to use. Now anyone who would like to use the phone number would have to manually copy it or type it.
| a { | ||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
I guess this is part of a reset, but generally it's nice to have some visual separation between regular text and links, which doesn't rely only on colour. Text decoration in the form of underline is perfect for this, and that's how most people expect links to behave - to have an underline (:
| header { | ||
| background-color: rgb(80, 156, 172); | ||
| } | ||
|
|
||
| main{ | ||
| background-color: rgb(26, 115, 138); | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| section { | ||
| margin: 20px auto; | ||
| border: solid 2px rgb(223, 218, 231); | ||
| border-radius: 10px; | ||
| padding: 10px; | ||
| font-size: 18px; | ||
| } |
There was a problem hiding this comment.
It's better to create class selectors and style them, instead of directly HTML elements. This is a better practice, especially when you get to have many more HTML files and you might not want to have the same style on all <section>, <head> or <main> tags.
| #summary{ | ||
| color: white; | ||
| font-size: 20px; | ||
| } |
There was a problem hiding this comment.
Avoid using ID selectors in CSS. IDs are preferred for selecting and manipulating elements through JavaScript. You can read here more about pros and cons of using Classes vs IDs - https://medium.com/@clairecodes/reasons-not-to-use-ids-in-css-a42204fb0d97
| margin-top: 0.25cm; | ||
| margin-left: 0.1cm; |
There was a problem hiding this comment.
It's best to use px, %, rem, vw, or vh.
CM is really not popular on websites.
| float: left; | ||
| } | ||
|
|
||
| #portfolio{ |
There was a problem hiding this comment.
Would be good if you can change this and all other ID selectors with class selectors.
|
Thank you Yason for your thorough review. I will fix the issues as soon as possible. |
There was a problem hiding this comment.
Interesting approach by including Bootstrap. However, I would have preferred if you have tried to build the page completely by yourself instead. Bootstrap is useful for big prototypes and quick development, but in most cases, you would be asked to build things completely from scratch. That's also the more optimal way, as Bootstrap comes with many lines of code, that rarely you would use all of.
If you have the time, please, see if you can rebuild the project by creating your own class names and styling properties. It could be a good exercise.
There was a problem hiding this comment.
A big part of the Front-end Development craft is connected to performance optimization. This is not only related to the code, but often to all other assets and files. The images can be optimized in multiple ways.
- they can be resized in a more fitting way. You would avoid loading a 1200 x 1200 px image in a container that might be just 250 x 250 px
- they can be compressed
- they can be exported in a more efficient format as say WebP
To achieve some of those improvements you can explore tools as TinyPNG and Squoosh.
| <link | ||
| rel="stylesheet" | ||
| href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css" | ||
| /> |
There was a problem hiding this comment.
This is a very big CSS library loaded for a tiny page. It might not be the best approach. It would be better to style everything on your own instead.
| <div class="col">Total</div> | ||
| <div class="col text-end">$333 / month</div> | ||
| </div> | ||
| <div><hr /></div> |
There was a problem hiding this comment.
This can be deleted and instead you could add border-bottom: 2px solid white; on the previous div.
| <div class="row fs-4 px-4 pb-4"> | ||
| <div class="col">Total</div> | ||
| <div class="col text-end">$100</div> | ||
| </div> |
There was a problem hiding this comment.
That's a lot of <div>. You could use instead a paragraph with 2 span tags inside.
| <div class="row fs-4 px-4 pb-4"> | |
| <div class="col">Total</div> | |
| <div class="col text-end">$100</div> | |
| </div> | |
| <p class="row fs-4 px-4 pb-4"> | |
| <span class="col">Total</span> | |
| <span class="col text-end">$100</span> | |
| </p> |
| <!-- Payment Method Part --> | ||
|
|
||
| <div class="payment container-fluid"> | ||
| <div class="display-6 fw-bold text-center p-5">Payment Method</div> |
There was a problem hiding this comment.
This could be an <h2> or <h3>. Don't wrap everything in a div.
| <div class="help text-center mb-5 fs-3"> | ||
| <span>Need Help?</span> | ||
| </div> |
There was a problem hiding this comment.
This most probably would be a <a> tag, judging by the text.
|
|
||
| </section> | ||
|
|
||
| <script src="https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/js/bootstrap.bundle.min.js"></script> |
There was a problem hiding this comment.
Is this used? Does your page still work without it?
| background-image: linear-gradient(to right, rgb(255, 94, 0), #e71bee); | ||
| color: white; | ||
| } | ||
| div:has(> hr) { |
There was a problem hiding this comment.
This is a very complex selector. Instead you could have a custom class for one of the rows above or below it with border-bottom: 2px solid white;.
| background-color: white; | ||
| } | ||
|
|
||
| .payment > div > div > div { |
There was a problem hiding this comment.
Please, have a read about selector specificity and see how you could use class names instead. It will help you significantly, especially once you start working with a bit bigger tasks and projects
🙂
Selectors' specificity W3Schools - https://www.w3schools.com/css/css_specificity.asp
Selectors' specificity MDN web docs - https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity
| background-clip: text; | ||
| } | ||
| div.help span:hover{ | ||
| cursor: pointer; |
There was a problem hiding this comment.
You wouldn't need to set the cursor if the tag was an <a> 😉
AliNematollahi-HackYourFuture
force-pushed
the
html-css/Ali
branch
from
0d7303d to
44d33fd
Compare
Homework files uploaded