-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Html css session day 3 #1
Conversation
.github/workflows FOLDERS were CREATED workflows goes inside .github linters.yml FILE was CREATED inside workflows folder linters.yml HTML & CSS configuration
1 "CONTAINER" DIV WAS CREATED 3 DIVs WERE CREATED INSIDE THE CONTAINER 5 CLASSES WERE CREATED "CONTAINER" , "ICON" , "NAME" , "PROFILE" , "LEFTMARGIN"
"container" , "icon" , "name" , "profile" , "leftmargin" CLASSES WERE CREATED
Changed the indentation
Expected indentation of 2 spaces
Unexpected extra semicolon
Added semicolon 8:2
Unexpected extra semicolon no-extra-semicolons 10:1 ✖ Unexpected extra semicolon no-extra-semicolons 16:1 ✖ Unexpected extra semicolon no-extra-semicolons 25:1 ✖ Unexpected extra semicolon no-extra-semicolons 38:1 ✖ Unexpected extra semicolon no-extra-semicolons
Changed the "leftmargin" & "icon" classes indentation let them in 1 line not in 3
No space between classes
Missed semicolon 11:21
11:21 Missed semicolon
7:15 Expected a trailing semicolon declaration-block-trailing-semicolon 10:1 Expected empty line before rule rule-empty-line-before 16:1 Expected empty line before rule rule-empty-line-before
16:2 Unexpected whitespace at end of line no-eol-whitespace 17:1 Unexpected empty line before closing brace block-closing-brace-empty-line-before 18:1 Expected empty line before rule rule-empty-line-before
17:1 Expected empty line before rule rule-empty-line-before 18:2 Unexpected whitespace at end of line no-eol-whitespace 19:3 Unexpected empty line before declaration declaration-empty-line-before
Update License Link [MIT license](https://github.com/gealsanchez/portfolio/blob/htmlCSS-Session-Day-3/LICENSE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gealsanchez ,
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!
Highlights 💯
- Professional-looking readme file. ✔️
- Clear and descriptive Pull Request. ✔️
- Clean and tidy code. ✔️
Required Changes ♻️
*** Kindly, I suggest you work a little more on your design, currently I can't tell which template you are basing on Figma
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
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<link rel="stylesheet" href="styles.css"> | ||
<title>Document</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly, I suggest you put a descriptive title to your website
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job following the advice of the previous reviewer
index.html
Outdated
|
||
<div class="container"> | ||
<div><img class="icon" src="geometry.png" alt=""></div> | ||
<div class="name leftmargin">Gerson Sanchez</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly, I suggest you use an H1 tag for the main header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job following the advice of the previous reviewer
index.html
Outdated
<div class="container"> | ||
<div><img class="icon" src="geometry.png" alt=""></div> | ||
<div class="name leftmargin">Gerson Sanchez</div> | ||
<div class="profile leftmargin">Hello! I am a software developer! I can help you build a product, feature or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kindly suggest using a "P" tag for traditional information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job following the advice of the previous reviewer
styles.css
Outdated
flex-direction: column; | ||
flex-wrap: wrap; | ||
border: 10px solid blue; | ||
width: 320px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly, I suggest you not to use fixed widths (it is preferable to use percentages) so that your website adapts to any size of mobile device.
index.html
Outdated
<body> | ||
|
||
<div class="container"> | ||
<div><img class="icon" src="geometry.png" alt=""></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly, I suggest you use a HEADER tag, and within it a NAV tag to contain the "hamburger" menu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly, I suggest you check this image link because the image does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job following the advice of the previous reviewer
Update the document title Professional Portfolio GERSON SANCHEZ
Changed the width from px to % Font families was setup
geometry.png added
"navigation" , "menu" CLASSES WERE ADDED hamburguer.png ICON line WAS ADDED
hamburguer.png WAS UPLOADED
"navigation" , "menu" CLASSES WERE DECLARED
13:11 ✖ Expected single space before "{" block-opening-brace-space-before 22:12 ✖ Expected a leading zero number-leading-zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STATUS: Required Changes ♻️♻️♻️
Hi @gealsanchez
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!
Highlights
✅ No linter error
✅ README is professional
Required Changes ♻️
- Kindly make sure you chose one temple to the Figma and your design must stick to the template design you have chosen as much as possible (e.g. font, colors, images, text, margins) using the templates in Figma. 😀 😀 😀
For example if you chose a template one your design will look at the picture below
the toolbar (or header):
the headline section (right after the header):
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 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="container"> | ||
<div><img class="icon" src="geometry.png" alt=""></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Kindly make sure the container of this section is a
header
tag.
Typically the header tag is used as a page header, it contains the site's title or logo (or both) and the main site navigation. Check this link for more info. 😀 😀 😀 -
The logo (if the design has one) is on the left and has a link (
a
tag).- This is for three reasons: convention, semantics, and user experience(UX). Check this link for more info. 😀 😀 😀
-
The hamburger button is on the right and has a
nav
tag.- The
nav
tag is for semantics reasons, and the right position for convention and user experience for mobile devices. 😀 😀 😀
- The
-
check the example below 👍
<header>
<a href="index.html">
<img src="images/logo.png" alt="Logo" />
</a>
<nav class="hamburger-menu">
<div class="bar-top"></div>
<div class="bar-middle"></div>
<div class="bar-bottom"></div>
</nav>
</header>
Kindly make sure you chose one temple to the Figma and your design must stick to the template design you have chosen as much as possible (e.g. font, colors, images, text, margins) using the templates in Figma.
Kindly make sure you chose one temple to the Figma and your design must stick to the template design you have chosen as much as possible (e.g. font, colors, images, text, margins) using the templates in Figma.
23:12 ✖ Expected a leading zero number-leading-zero 46:3 ✖ Unexpected empty line before declaration declaration-empty-line-before
width: 50%
Placed the right image in the right bottom side
Added a div to align the social media links and the image of the left side
Duplicated lines deleted
54:8 ✖ Expected single space before "{" block-opening-brace-space-before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gealsanchez ,
Great work on making the changes requested by a previous reviewer 👏
You've done well implementing some of the requested changes, but there are still some which aren't addressed yet.
Highlights 💯
- Good job following the advice of the previous reviewer ✔️
Required Changes ♻️
- Kindly, I suggest you check a white border and a horizontal scroll that makes your website not look very good, please check what element produces this and adjust it
- Kindly, I suggest you revise the background image and social icons, so that it looks similar to the original Figma design.
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
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.
Social media and images were uploaded
body margin 0%
I forgot to upload the images my mistake sorry |
No more needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STATUS: APPROVED ✔️ 🟢
Hi @gealsanchez 😄 ,
Your project is complete! There is nothing else to say other than... it's time to merge it
Congratulations! 🎉
Highlights ✨
Optional suggestions ☑️
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
APPROVED! 💯 💯 💯
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me, @Nyame-Wolf 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.
display: flex; | ||
flex-direction: column; | ||
border-radius: 2%; | ||
width: 375px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[OPTIONAL]I suggest getting rid of this fixed width. The app looks good but can serve only devices that have a minimum width of 375 px while its meant to serve devices between 768px and 375px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Nyame-Wolf how do you describe that behavior, dynamic width?
<div class="container"> | ||
<header> | ||
<nav class="navigation"> | ||
<p>John Doe</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[OPTIONAL] I suggest making this logo a link element <a>
instead of a paragraph.
- The logo (if the design has one) is on the left and has a link (a tag).
This is for three reasons: convention, semantics, and user experience(UX). Check this link for more info.
<body> | ||
|
||
<div class="container"> | ||
<header> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.github/workflows FOLDERS were CREATED workflows goes inside .github
linters.yml FILE was CREATED inside workflows folder
linters.yml HTML & CSS configuration
1 "CONTAINER" DIV WAS CREATED
3 DIVs WERE CREATED INSIDE THE CONTAINER
5 CLASSES WERE CREATED "CONTAINER" , "ICON" , "NAME" , "PROFILE" , "LEFTMARGIN"
"container" , "icon" , "name" , "profile" , "leftmargin" CLASSES WERE CREATED
linters.yml errors corrected
Optional Suggestions**
Update the document title Professional Portfolio GERSON SANCHEZ
Changed the width from px to %
Font families was setup
geometry.png added
"navigation" , "menu" CLASSES WERE ADDED
hamburguer.png ICON line WAS ADDED
hamburguer.png WAS UPLOADED
"navigation" , "menu" CLASSES WERE DECLARED
Linters.yml errors
13:11 heavy_multiplication_x Expected single space before "{" block-opening-brace-space-before
22:12 heavy_multiplication_x Expected a leading zero number-leading-zero
Required change
Kindly make sure you chose one temple to the Figma and your design must stick to the template design you have chosen as much as possible (e.g. font, colors, images, text, margins) using the templates in Figma.