Conversation
mckent05
left a comment
There was a problem hiding this comment.
Hi @SSEKPIUS,
STATUS : CHANGES REQUIRED ♻️ ♻️
Good job so far!
You still need to work on some issues to go to the next project, but you are almost there!
Highlights
- All linter checks are passing correctly ✔️ ✔️
- GitHub flow was used correctly 👏 👏
Required Changes ♻️
- Please apply all suggested changes in the
index.htmlpage as they ay apply in theabout.htmlpage. 👍 👍 - Also, apply all changes in the desktop version as they may apply in the mobile version 😄 😄
Check the comments under the review.
Optional suggestions
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.
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.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
index.html
Outdated
| <div class="cover-darker"> | ||
| <div class="container"> | ||
| <ul class="top-tag"> | ||
| <li><i class="fa fa-facebook" aria-hidden="true"></i></li> | ||
| <li><i class="fa fa-twitter" aria-hidden="true"></i></li> | ||
| <li>Logout</li> | ||
| <li>Mypage</li> | ||
| <li>Korean</li> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| <div class="cover-white"> | ||
| <div class="container"> | ||
| <div class="top-nav"> | ||
| <a class="logo" href="/#"> | ||
| <img src="images/logo.jpeg" alt="logo"> | ||
| </a> | ||
| <div class="humberger" onclick="toggleMobileMenu()"> | ||
| <div></div> | ||
| <div></div> | ||
| <div></div> | ||
| </div> | ||
| <nav> | ||
| <ul> | ||
| <li class="active"> | ||
| <a href="/">Home</a> | ||
| </li> | ||
| <li> | ||
| <a href="/about.html">About</a> | ||
| </li> | ||
| <li> | ||
| <a class="nav-link" href="#Program">Program</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link" href="#Join">Join</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link" href="#Sponsor">Sponsor</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link disabled" href="#News">News</a> | ||
| </li> | ||
| </ul> | ||
| </nav> | ||
| <div class="humberger-close" | ||
| onclick="toggleMobileMenu()"> | ||
| <i class="fa fa-times fa-2x" aria-hidden="true"></i> | ||
| </div> | ||
| <div class=" tag"> | ||
| <span> | ||
| live aware | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
- Good job done so far 👏 👏. To give your website a better HTML structure and to improve the accessibility of your site, I'd advise you to wrap the whole of this section in a
navtag. Please fix this 👍 👍
index.html
Outdated
| <a class="logo" href="/#"> | ||
| <img src="images/logo.jpeg" alt="logo"> | ||
| </a> | ||
| <div class="humberger" onclick="toggleMobileMenu()"> |
There was a problem hiding this comment.
- To make your HTML page clean and maintainable, it is not advisable to add inline JS to your HTML page, I'd suggest you keep all JS implementation in your JS file. Please fix this 👍 👍
index.html
Outdated
| <nav> | ||
| <ul> | ||
| <li class="active"> | ||
| <a href="/">Home</a> | ||
| </li> | ||
| <li> | ||
| <a href="/about.html">About</a> | ||
| </li> | ||
| <li> | ||
| <a class="nav-link" href="#Program">Program</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link" href="#Join">Join</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link" href="#Sponsor">Sponsor</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link disabled" href="#News">News</a> | ||
| </li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
- Since you'd wrap the whole of this section in a nav tag, you would not need another
navtag here, I'd suggest you remove this 🙏 🙏 . - I cannot properly access the pages in your website, this is because you have included the wrong links in your page. I suggest you remove the
/included in your href. 👍 👍. - Your bottom nav does not exactly look like the one in the design template, Remember it is important that you try to replicate the Figma design as closely as possible to give confidence to recruiters in your ability to produce exact business requirements 🙏
Your Project 👇:
- Please pay attention to the difference in font size, I'd suggest you reduce the font size of your nav links because they make your navbar take up a lot of space, you could also consider reducing the size of your logo.
Design Template:
- Since you made your nav fixed, other parts of your website seem to overflow on your nav, please see the picture attached below for reference, I suggest you read this article on tips to fix this issue 👍 👍
Your nav:
index.html
Outdated
| </ul> | ||
| </nav> | ||
| <div class="humberger-close" | ||
| onclick="toggleMobileMenu()"> |
There was a problem hiding this comment.
- Just as i mentioned earlier, please keep all JS implementation in your JS file 👍 👍
index.html
Outdated
| <h3 class="greeting">"Hello! Sharing World"</h3> | ||
| <h1>live aware global summit 2022</h1> |
There was a problem hiding this comment.
- To give your HTML page a better structure and for improved accessibility, it is important that the
headertags in your page follow the right hierarchy. i.e thehxtags are arranged in ascending order. I'd suggest you use aptag in place of theh3tag
| <section> | ||
| <div class="landing-page"> | ||
| <div class="container"> | ||
| <div> | ||
| <h3 class="greeting">"Hello! Sharing World"</h3> | ||
| <h1>live aware global summit 2022</h1> | ||
| <p class="about">A joyful celebration believing in the value of openness and sharing, | ||
| creating a positive change with poeple from all over 100 countries is taking place in October | ||
| ,in korea. | ||
| </p> | ||
| <p class="date">2022.10.17(THUR) ~ 30(FRI)</p> | ||
| <p class="location">@ National Museum of Korea, Art Center Nabi and more</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
- This section of the page doesn't exactly look like the given design template, please try to fix this 🙏 . Remember it is important that you try to replicate the Figma design as closely as possible to give confidence to recruiters in your ability to produce exact business requirements 🙏
Your Project:
-- While it is important to personalize the content of this project, it is important you stick to the general design layout (background images, color, margins, padding, font size) as seen in the design template
-- Some parts of this section aren't visible because your navbar is overflowing, please try to fix this issue 🙏 🙏 .
-- Some parts of this section are overflowing into the next section (Main program). Please fix this issue 👍 👍
Design Template:
index.html
Outdated
| <div class="main-program"> | ||
| <div> | ||
| <h2>main program</h2> | ||
| <p class="underline"></p> | ||
| </div> | ||
| <div class="cards"> | ||
| <div class="card"> | ||
| <i class="fa fa-podcast fa-3x" | ||
| aria-hidden="true"></i> | ||
| <h3>lecture</h3> | ||
| <p>Diam justo et amet sadipscing accusam duo et. | ||
| Sea ipsum lorem est sadipscing ipsum est | ||
| duo.</p> | ||
| </div> | ||
| <div class="card"> | ||
| <i class="fa fa-shopping-bag fa-3x" | ||
| aria-hidden="true"></i> | ||
| <h3>CC exhibition</h3> | ||
| <p>Seemed and charms cheer from for he, to true | ||
| one amiss maidens whose mine below dares..</p> | ||
| </div> | ||
| <div class="card"> | ||
| <i class="fa fa-forumbee fa-3x" | ||
| aria-hidden="true"></i> | ||
| <h3>forum</h3> | ||
| <p>Und folgenden michjenem folgenden ihr euch | ||
| dunst ist fühlt busen, fühl die.</p> | ||
| </div> | ||
| <div class="card"> | ||
| <i class="fa fa-building fa-3x" | ||
| aria-hidden="true"></i> | ||
| <h3>workshop</h3> | ||
| <p>Fyodumtul nym hioll de um yg symeonnok | ||
| therthetyk vh ualallal uiraga werethul | ||
| therthetyk. Urodum.</p> | ||
| </div> | ||
| <div class="card"> | ||
| <i class="fa fa-free-code-camp fa-3x" | ||
| aria-hidden="true"></i> | ||
| <h3>CC ignite</h3> | ||
| <p>Kiun sakon tien-cxi metitan veloj la povis | ||
| sendube gastoj sian sed, ridetis en knabo | ||
| pri la sxipo. Tiam tiom bestoj.</p> | ||
| </div> |
There was a problem hiding this comment.
- For better semantics and to improve your site's accessibility, I'd suggest you use an
articletag for each of your tags, please fix this 👍 👍 - In your mobile version, the content of the cards aren't properly aligned, please fix this issues 🙏 🙏
Your project(Mobile version):
Design Template:
| <section> | ||
| <div class="cover-darker desktop-hide"> | ||
| <div class="container"> | ||
| <div class="partners"> | ||
| <div> | ||
| <h2>partner</h2> | ||
| <p class="underline"></p> | ||
| </div> | ||
| <div> | ||
| <img src="images/mozilla.png" alt="mozilla"> | ||
| <img src="images/google.png" alt="google"> | ||
| <img src="images/naver.png" alt="naver"> | ||
| <img src="images/kakao.png" alt="kakao"> | ||
| <img src="images/bnb.png" alt="airbnb"> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
-This section is not present on the about page in the design template, I'd suggest you remove it 👍 👍. Remember it is important that you try to replicate the design template as closely as possible to give confidence to recruiters in your ability to produce exact business requirements 🙏
Design template:
There was a problem hiding this comment.
@mckent05 , actually this is part of the about page, but as you recommended, ill have it removed asap.
There was a problem hiding this comment.
- Hello @SSEKPIUS, I was referring to the 'sponsor' section, that's the part I highlighted in your code 😄. I only attached the picture of the design template as a reference.
index.js
Outdated
| return true; | ||
| }); | ||
| } | ||
| showSpeakers(); |
There was a problem hiding this comment.
- In order to make your JS code clean and maintainable, I'd advise you to call and attach the function to an event listener like
DOMContentLoaded. Please try to fix this 👍 👍
mckent05
left a comment
There was a problem hiding this comment.
Hi @SSEKPIUS,
STATUS: CHANGES REQUIRED ♻️ ♻️
Good job so far!
You still need to work on some issues to go to the next project, but you are almost there!
Highlights
- All linter checks are passing ✔️ ✔️
- GitHub flow is correctly used 👍
Required Changes ♻️
Check the comments under the review.
Optional suggestions
OPTIONAL - I missed this in my last review, but it is very important 🙏. Please deploy your project on GitHub pages 👍 👍. It would help make your repo more professional and make it easy to view your project.
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.
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.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
index.html
Outdated
| <div class="program"> | ||
| <div class="icon"> | ||
| <img src="images/program_icon_01.png" | ||
| alt="program"> | ||
| </div> | ||
| <div class="title"> | ||
| <span>conference</span> | ||
| </div> | ||
| <div class="text"> | ||
| <span> | ||
| Listen to lectures from speakers from | ||
| around the world and learn about the | ||
| latest trends in the world. | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <div class="program"> | ||
| <div class="icon"> | ||
| <img src="images/program_icon_02.png" | ||
| alt="program"> | ||
| </div> | ||
| <div class="title"> | ||
| <span>CC exhibition</span> | ||
| </div> | ||
| <div class="text"> | ||
| <span> | ||
| Let's meet the creations of artists from | ||
| various fields who share the open spirit | ||
| of CC. | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <div class="program"> | ||
| <div class="icon"> | ||
| <img src="images/program_icon_03.png" | ||
| alt="program"> | ||
| </div> | ||
| <div class="title"> | ||
| <span>forum</span> | ||
| </div> | ||
| <div class="text"> | ||
| <span> | ||
| We have time to share our thoughts and | ||
| opinions with experts by topic. | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <div class="program"> | ||
| <div class="icon"> | ||
| <img src="images/program_icon_04.png" | ||
| alt="program"> | ||
| </div> | ||
| <div class="title"> | ||
| <span>workshop</span> | ||
| </div> | ||
| <div class="text"> | ||
| <span> | ||
| You can create your own creations using | ||
| open source instead of just looking at | ||
| them. | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <div class="program"> | ||
| <div class="icon"> | ||
| <img src="images/program_icon_05.png" | ||
| alt="program"> | ||
| </div> | ||
| <div class="title"> | ||
| <span>CC party</span> | ||
| </div> | ||
| <div class="text"> | ||
| <span> | ||
| Create opportunities to freely network | ||
| with CC personnel from around the world. | ||
| </span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
- Just as I suggested in my last review, for semantic reasons, I suggest you wrap each of your program cards in an
articletag. please try to fix this 👍 👍
| <footer> | ||
| <div class="cover-white cover-darker-desktop"> | ||
| <div class="container"> | ||
| <div> | ||
| <a href="/"> | ||
| <img src="images/logo_cck_w_208x35.png" alt="logo"> | ||
| </a> | ||
| <div> | ||
| <span><b>2015 Creative Commons Korea. Some Rights Reserved.</b></span> | ||
| <p>Lorem ipsum dolor sit amet, consectetur | ||
| adipisicing elit. | ||
| Eaque sed in perferendis, quod aperiam optio | ||
| maiores omnis ipsum, | ||
| et pariatur tempore fuga quisquam reprehenderit | ||
| veritatis.</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
- Please include the section attached in the picture referenced below in your design. I noticed you removed it (due to a misunderstanding in my last review) 🙏 🙏
OPTIONAL - In my last review I asked that you removed the sponsor section, I should have mentioned that it should be removed only in the desktop version only. It would be nice if you could implement this 😄
ichala
left a comment
There was a problem hiding this comment.
Hi @SSEKPIUS ,
Status: Approved ✔️ ✔️ ✔️
To Highlight 💪🏻
- Changes Implemented ✔️
Good job implementing the required changes 👍 Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉
Optional Suggestions :
- N/A
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.
Please, remember to tag @ichala in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
About:
https://www.loom.com/share/affb3790c7084351806750362f23ead2