-
Notifications
You must be signed in to change notification settings - Fork 16
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
htmlcss/nina #7
base: main
Are you sure you want to change the base?
htmlcss/nina #7
Conversation
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.
Very well done Nina, I am honestly very impressed with the quality of the code and how great your portfolio page already looks. I don't have any significant suggestions on how to improve the code or the result 🥇 nice detail with your profile picture overlapping the top menu, makes it more interesting visually.
I am only missing a netflify link for the homework. I only saw you share for the class exercise.
When you raise a Pull Request (PR) you can add a description. Here you would typically add a few words about what changes the PR includes. It can help the reviewer a lot, so make it a habit so also do this 👍
For instance it could be something like:
This is my homework for HTML/CSS week 1. It contains both our class exercise and the homework. I struggled with the second part where... I tried to do like... but it did not work... 😢
Class exercise: https://nina-order-payment2.netlify.app/
Homework: https://nina-some-other-link.netlify.app/
All in all, very good work! keep it up 💪🔥
Here are some ideas for you portfolio if you decide to work more one it:
- Making you website more accessible using ARIA in your HMTL
- Make your top header stick to the top of the screen when the user scrolls down the page
<path | ||
d="M24 12C24 18.6274 18.6274 24 12 24C5.37258 24 0 18.6274 0 12C0 5.37258 5.37258 0 12 0C18.6274 0 24 5.37258 24 12Z" | ||
fill="#5D616A" /> | ||
<path | ||
d="M13.0986 14.6777H10.8574C10.8516 14.3555 10.8486 14.1592 10.8486 14.0889C10.8486 13.3623 10.9688 12.7646 11.209 12.2959C11.4492 11.8271 11.9297 11.2998 12.6504 10.7139C13.3711 10.1279 13.8018 9.74414 13.9424 9.5625C14.1592 9.27539 14.2676 8.95898 14.2676 8.61328C14.2676 8.13281 14.0742 7.72266 13.6875 7.38281C13.3066 7.03711 12.791 6.86426 12.1406 6.86426C11.5137 6.86426 10.9893 7.04297 10.5674 7.40039C10.1455 7.75781 9.85547 8.30273 9.69727 9.03516L7.42969 8.75391C7.49414 7.70508 7.93945 6.81445 8.76562 6.08203C9.59766 5.34961 10.6875 4.9834 12.0352 4.9834C13.4531 4.9834 14.5811 5.35547 15.4189 6.09961C16.2568 6.83789 16.6758 7.69922 16.6758 8.68359C16.6758 9.22852 16.5205 9.74414 16.21 10.2305C15.9053 10.7168 15.249 11.3789 14.2412 12.2168C13.7197 12.6504 13.3945 12.999 13.2656 13.2627C13.1426 13.5264 13.0869 13.998 13.0986 14.6777ZM10.8574 18V15.5303H13.3271V18H10.8574Z" |
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.
This is one way of making SVG icons. I assume you got it from a template or generated from AI. It is okay to have but it can be hard to understand the code. For this reason most company will use icon libraries such as Font awesome. You will most likely be introduced to this in one of the later modules. 👍
</div> | ||
</div> | ||
<!-- submit button --> | ||
<div class="submit-button"> |
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.
it might be a little confusing having the class named submit-button
when it is not actually used on the <input>
or <button>
. You could consider calling it submit-button-container
instead.
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.
Well done, Nina
The Order task looks especially well. You can improve a bit on some areas as using more class names and semantic tags, but overall you are on a good track.
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.
Those assets are really big in file size. You can look into optimizing them through a tool like TinyPNG or Squoosh. I could recommend you exporting them in WebP format. Also if you have the possibility resize them down. For example, it's unnecessary to load a 1200 x 1200 px file on a 250 x 250 px box.
/* var(--gradient) */ | ||
/* :root { | ||
--gradient: your gradient color here; | ||
} */ |
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.
Remember to delete commented-out code.
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.
This image can significantly affect the performance of your website, due to the large file size. Please, follow the advice from the previous comment on optimizing with TinyPNG or Squoosh.
It can easily be made as 80 KB (55 times file reduction).
Here is it as a zip - GitHub doesn't allow upload of WebP in the comments.
Nina-Ruoyin-Lin.webp.zip
<a href="#projects"> Projects </a> | ||
</li> | ||
<li> | ||
<a href="#contact" class="button contact-me-button">Contact Me</a> |
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.
You can delete the .contact-me-button
and the fixed height
on .button
. There will always be a resolution on which one or another button will break on multiple lines.
<main> | ||
<section class="hero container"> | ||
<div class="hero-pink"> | ||
<img src="./imgs/hero-img.png" alt="Nina Lin" width="100%" /> |
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.
<img src="./imgs/hero-img.png" alt="Nina Lin" width="100%" /> | |
<img src="./imgs/hero-img.png" alt="Nina Lin" /> |
The image default width is always 100% as a block element.
Very good practice with adding alt
attribute.
margin-top: 30px; | ||
} | ||
|
||
hr { |
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.
As I've commented in the HTML, you can replace this with border-bottom: 2px solid white;
on the preceding element.
.ordinary-summary { | ||
width: 200px; | ||
height: 40px; | ||
font-family: 'Arial'; |
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.
This could be set on body
if it's expected to be the default font-family on the page.
line-height: 22px; | ||
display: flex; | ||
padding-left: 10px; | ||
padding-right: 10px; |
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.
Add more padding to the right to prevent input content overlap with the ?
icon.
flex-direction: column; | ||
} | ||
|
||
#buy-now { |
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.
Avoid styling on ID
- https://medium.com/@clairecodes/reasons-not-to-use-ids-in-css-a42204fb0d97
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.
May be this style could be called something as .primary-button
in case you want to use it elsewhere. Now it's very specific to the content it has.
font-family: 'Arial'; | ||
font-style: normal; | ||
font-weight: 700; | ||
font-size: 18px; | ||
line-height: 21px; | ||
display: flex; | ||
align-items: center; | ||
text-align: center; | ||
justify-content: center; |
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.
Please, don't copy all code from Figma. There are a lot of properties that either can be inherited from a parent element, or are unnecessary.
No description provided.