search

Htetaungkyaw71 / Bootstrap-portfolio-peer-programming

This is Portfolio project with bootstrap
1 stars 0 forks source link

Need to review and you can suggest for my bootstrap project (6 Oct) #2

Open Htetaungkyaw71 opened 2 years ago

Htetaungkyaw71 commented 2 years ago

https://github.com/Htetaungkyaw71/Bootstrap-portfolio-peer-programming/blob/f48c0c1165b92973ea664ab4ff0c357e6a0d31ba/index.html#L1-L299

akhror-valiev commented 2 years ago

Hello @Htetaungkyaw71

abdallahmalima commented 2 years ago

Changes Required ♻️ Hi @Htetaungkyaw71 Nice job, you are almost there.

(Highlights) Good Points: : Good implementation of some of the project requirements ✔ GitHub flow was correctly used ✔. Good design 🔥🔥 (Changes Required) Aspects to improve: ♻️ You did great implementing the project requirements, but you still have some issues: Check the comments below 👇

[Optional] suggestions: Nothing to mention. Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

Cheers and Happy coding!👏👏👏

Feel free to leave any questions or comments in the PR thread if something is not 100% clear, and please remember to tag me in your question so I can receive the notification**

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.

Kindly replace the first section with a header tag since it contains navigation tag,this is strongly recommended as per HTML Semantics best practice. instead of what you did you can do like this:

<header> 
         <div class="container"> 
             <nav class="navbar navbar-expand-lg navbar-light "> 
                 <div class="container-fluid"> 
                   <a class="navbar-brand text-primary" href="#">My Logo</a> 
                   <button class="navbar-toggler" type="button" data-bs-toggle="collapse" data-bs-target="#navbarSupportedContent" aria-controls="navbarSupportedContent" aria-expanded="false" aria-label="Toggle navigation"> 
                     <span class="navbar-toggler-icon"></span> 
                   </button> 
                   <div class="collapse navbar-collapse" id="navbarSupportedContent"> 
                         <ul class="navbar-nav ms-auto mb-2 mb-lg-0"> 
                             <li class="nav-item"> 
                               <a class="nav-link active" aria-current="page" href="#">Portfolio</a> 
                             </li> 
                             <li class="nav-item"> 
                               <a class="nav-link active" href="#">Work</a> 
                             </li> 
                             <li class="nav-item"> 
                                 <a class="nav-link active" href="#">Contact</a> 
                               </li> 
                         </ul> 
                   </div> 
                 </div> 
             </nav> 
         </div>  
     </header>