html-css #14
Open
html-css #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Just as the background.png, you could optimize this one, too.
There was a problem hiding this comment.
This asset is 1201 x 1209 pixels but you show it in a square which is close to 250 x 250 px. If you size the asset down to 250 or 500, you would get some good points in optimization with Google Lighthouse test ;)
There was a problem hiding this comment.
This asset (and maybe others) is not used anywhere on your site. Maybe you can delete it, to keep the project size small.
html-css/week1/Portfolio/index.html
Outdated
Comment on lines
37
to
39
| <div class="profile__picture"> | ||
| <img src="./img/neepun.jpg" alt="Neepun Raj" /> | ||
| </div> |
There was a problem hiding this comment.
Could you move the class name down to the img tag and remove the <div> tag?
Suggested change
| <div class="profile__picture"> | |
| <img src="./img/neepun.jpg" alt="Neepun Raj" /> | |
| </div> | |
| <img src="./img/neepun.jpg" alt="Neepun Raj" class="profile__picture" /> |
| opacity: 1 | ||
| } | ||
| } | ||
| .socials__github,.socials__email,.socials__linkedin { |
There was a problem hiding this comment.
YOu could use one class for all those, called .socials__icon and then wrap them in a div with flex box where you use the gap property
Comment on lines
529
to
532
| .contact_text{ | ||
| font-size: 1.8rem; | ||
| margin-bottom: 6rem | ||
| } |
There was a problem hiding this comment.
This overwrites the definition from a few lines up. I guess you can remove the one up there and leave this.
| font-weight: 700 | ||
| } | ||
| .contact input, .contact textarea { | ||
| width: 500px; |
There was a problem hiding this comment.
This will cause you horizontal scroller on devices below 500px width unless you change it with a media query.
| .contact input:focus,.contact textarea:focus { | ||
| outline: none | ||
| } | ||
| .contact input::placeholder,.contact textarea::placeholder { |
There was a problem hiding this comment.
Relying only on placeholder text is not good for web accessibility. It's preferred to use a <label> tag and to associate it to the input fields by using the for attribute.
| display: flex; | ||
| justify-content: space-between; | ||
| top: 0; | ||
| cursor: pointer; |
There was a problem hiding this comment.
I'd delete this, as it's causing confusion. pointer is used for clickable elements, but here it's on the entire navigation. It misleads the user that they can click the menu items from the padding space, but actually it works only when you click exactly on the text.
|
Wow, Thank you, will definetly make update on these corrections and
suggestions ,
Thank you
…On Sat, Jan 18, 2025, 18:18 Y. Vasilev ***@***.***> wrote:
***@***.**** commented on this pull request.
Hi, Neepun.
Super good design. I was pleasantly surprised to see that you use BEM,
animations, and CSS variables.
I've left you some suggestions here and there how to optimize a bit the
code or make the end result more accessible.
Great job!
------------------------------
On html-css/week1/Portfolio/img/background.png
<#14 (comment)>
:
This asset is quite big and could make the entire page load slow according
to some Google metrics. I'd recommend you parsing it through TinyPNG
<https://tinypng.com/> or Squoosh <https://squoosh.app/> and export it as
WebP. You can see up to 97% of size improvement.
------------------------------
On html-css/week1/Portfolio/img/learningApp.png
<#14 (comment)>
:
Just as the background.png, you could optimize this one, too.
------------------------------
On html-css/week1/Portfolio/img/neepun.jpg
<#14 (comment)>
:
This asset is 1201 x 1209 pixels but you show it in a square which is
close to 250 x 250 px. If you size the asset down to 250 or 500, you would
get some good points in optimization with Google Lighthouse test ;)
------------------------------
On html-css/week1/Portfolio/img/portfolio.png
<#14 (comment)>
:
This asset (and maybe others) is not used anywhere on your site. Maybe you
can delete it, to keep the project size small.
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <div class="profile__picture">
+ <img src="./img/neepun.jpg" alt="Neepun Raj" />
+ </div>
Could you move the class name down to the img tag and remove the
tag?
⬇️ Suggested change
- <div class="profile__picture">
- <img src="./img/neepun.jpg" alt="Neepun Raj" />
- </div>
+ <img src="./img/neepun.jpg" alt="Neepun Raj" class="profile__picture" />
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <li><a href="#about">About</a></li>
+ <li><a href="#projects">Projects</a></li>
+ <li><a href="#contacts">Contact</a></li>
+ </ul>
+ </nav>
+ </header>
+ <main>
+
+ <section class="hero container ">
+ <h1>Transform <br />Your Idea into <br />Action</h1>
+ <p>Hi, I am Neepun Raj your next Software Engineer</p>
+ <h2>Full Stack Developer</h2>
+ </section>
+
+ <section class="about" id="about">
+ <h2 class="about__heading section-heading">About</h2>
Super nice to see that you are familiar with BEM 🤩
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <div class="skills__row">
+ <div class="skills__item ">
+ <img src="./img/html.png" alt="html" />
+ <div class="skills__item-name">HTML</div>
+ </div>
+ <div class="skills__item ">
+ <img src="./img/react.png" alt="" />
+ <div class="skills__item-name">REACT</div>
+ </div>
+ <div class="skills__item ">
+ <img src="./img/express.png" alt="" />
+ <div class="skills__item-name">EXPRESS.JS</div>
+ </div>
+ </div>
+ <div class="skills__row">
+ <div class="skills__item ">
+ <img src="./img/javascript.png" alt="" />
+ <div class="skills__item-name">JAVASCRIPT</div>
+ </div>
+ <div class="skills__item ">
+ <img src="./img/css.png" alt="" />
+ <div class="skills__item-name">CSS</div>
+ </div>
+ <div
+ class="skills__item "
+ >
+ <img src="./img/mongodb.png" alt="" />
+ <div class="skills__item-name">MONGODB</div>
+ </div>
Those resemble a list, but being wrapped in
s they might not be as semantic as they could be.
Suggestion:
<ul class="skills__row">
<li class="skills__item ">
<img src="./img/html.png" alt="html" />
<span class="skills__item-name">HTML</span>
</li>
<li class="skills__item">
<img src="./img/react.png" alt="" />
<span class="skills__item-name">REACT</span>
</li>
</ul>
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <div class="skills__item-name">NEXT.JS</div>
+ </div>
+ <div class="skills__item ">
+ <img src="img/nodejs.png" alt="" />
+ <div class="skills__item-name">NODE.JS</div>
+ </div>
+ </div>
+ </div>
+ </div>
+ </section>
+
+ <section class="projects" id="projects">
+ <h2 class="projects__heading section-heading">Projects</h2>
+ <div class="project project-left">
+ <a href="#">
+ <div class="project__image-container weather">
Does the weather class do anything?
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + margin-bottom: 6rem !important;
+}
+
+.contact_text{
+ font-size: 1.8rem;
+ margin-bottom: 6rem
+}
+.contact_form{
+ display: flex;
+ flex-direction: column;
+ align-items: center;
+ justify-content: center;
+ gap: 20px;
+
+}
+.contact__form-name, .contact__form-email{
You can use one class for both cases, instead of adding two.
Eg .contact__form-input.
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <a href="#hero">
+ <div class="return-home">
+ <svg
+ version="1.1"
+ xmlns="http://www.w3.org/2000/svg"
+ width="100%"
+ viewBox="0 0 24 24"
+ >
+ <title></title>
+ <path
+ fill="#fafafa"
+ d="M17.707 10.293l-5-5c-0.391-0.391-1.024-0.391-1.414 0l-5 5c-0.391 0.391-0.391 1.024 0 1.414s1.024 0.391 1.414 0l4.293-4.293 4.293 4.293c0.391 0.391 1.024 0.391 1.414 0s0.391-1.024 0-1.414zM17.707 17.293l-5-5c-0.391-0.391-1.024-0.391-1.414 0l-5 5c-0.391 0.391-0.391 1.024 0 1.414s1.024 0.391 1.414 0l4.293-4.293 4.293 4.293c0.391 0.391 1.024 0.391 1.414 0s0.391-1.024 0-1.414z"
+ ></path>
+ </svg>
+ </div
+ ></a>
Very nifty. Just remember to add something in the <title> tag. Eg. "Back
to the top of the page"
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <a href="https://github.com/neepunraj" target="_blank">
+ <div class="socials__github">
+ <img src="img/github.png" alt=""/>
+ </div>
You could add alt text and move the class from the div down to the
images. No need of the extra
wrapping.
------------------------------
In html-css/week1/Portfolio/index.html
<#14 (comment)>
:
> + <img src="img/github.png" alt=""/>
+ </div>
+ </a>
+ <a href="https://www.linkedin.com/in/neepunrajshrestha/" target="_blank">
+ <div class="socials__linkedin">
+ <img src="img/icons8-linkedin-100.png" alt=""/>
+ </div>
+ </a>
+ <a ***@***.***" target="_blank" >
+ <div class="socials__email">
+ <img src="img/mail.png" alt=""/>
+ </div>
+ </a>
+ </div>
+
+ <p class="copyright">Neepun Raj © 2025</p>
The recommended tag for legal copy in footers and similar disclaimers is
<small> -
https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-small-element
Nothing wrong in using, paragraph, too.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + }
+ .skills__item img {
+ height:45px
+ }
+ .project__info {
+ top:4rem
+ }
+}
+
***@***.*** (max-width: 500px) {
+ .skills__item img {
+ height:35px
+ }
+}
+
***@***.*** (max-width: 410px) {
Generally, there might be 3 media queries in a project. One for desktop
devices and screens, another for tablets, and then last one for phones.
Having more than 3 media queries might be hard to maintain in the long run
and would require a lot of time to develop. Sometimes using flex-wrap:
wrap on elements, or using fluid widths in % can help avoid having to
specify additional media queries.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> @@ -0,0 +1,686 @@
+:root {
+ --drk-blu: #7000c5;
+ --lt-blu: #daf9ff;
+ --colorText: linear-gradient(90deg, #ff66c4 40%, #ffdef9 60%);
This doesn't seem to be used anywhere.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + background-image: url('../img/background.png');
+ background-size: cover;
+ background-repeat:no-repeat;
+ background-position: center center;
This would apply the styling to all tags where this CSS file is included.
You might like to start another file with a different section. I'd
recommend that you move the image instead on the hero section. You can
move the container tag to a nested inside
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +
+li{
+ list-style: none;
+}
+a{
+ text-decoration: none;
+ color: inherit;
+}
+button {
+ border: none;
+ outline: none;
+ background-color: transparent;
+ color: inherit
+}
+.hero {
+ position: relative;
This and the z-index below don't seem to do anything.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + flex-direction: column;
+ height: 100vh;
+ width: 100%;
+ z-index: 999
+
+}
+.about{
+ width: 1300px;
+ margin-left: auto;
+ margin-right: auto;
+ display: flex;
+ flex-direction: column;
+ align-items: center;
+}
+
+.about__heading:before, .contact__heading:before, .projects__heading:before{
You could make a class called .section-title and have .section-title:after.
It will reduce the CSS you have.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + display: flex;
+ flex-direction: column;
+ align-items: center;
+}
+
+.about__heading:before, .contact__heading:before, .projects__heading:before{
+ position: absolute;
+ content: '';
+ border-bottom: 18px solid #ff4d5a;
+ width: 16rem;
+ display: block;
+ margin: 0 auto;
+ position: relative;
+ left: 3.5rem;
+ top: 6.3rem;
+ z-index: 9999;
This is the absolute maximum of z-index one can use. Imagine you have a
modal appear later on page for some reason. You won't be able to make it
appear on top of those underlines. It's recommended to add z-index from 1
up with 10 or 100 increments. For example, you can have elements like those
underlines that would be 1, then main menu if sticky, could be 10 or 100, a
modal 500 etc.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + display: block;
+ margin: 0 auto;
+ position: relative;
+ left: 3.5rem;
+ top: 6.3rem;
+ z-index: 9999;
+}
+.about__content{
+ display: flex;
+ justify-content: space-between;
+ width: 100%;
+}
+
+.main-header{
+ background-color: transparent;
+ color: #fff;
One of the defined variables at the top of the file is used only once, but
#fff is used 3 times. You can easily set it as a variable, do make future
updates to the colour easier.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +logo-img{
+ height: 50px;
+ width: 100px;
+}
There isn't such tag on the page. You can delete this.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +img {
+ overflow-clip-margin: content-box;
+ overflow: clip;
+}
Would be nice to keep global styles like this at the top of the file. It's
similar to reset CSS files - they are always loaded first.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + letter-spacing: 1px;
+}
+.skills__item img{
+ height: 60px;
+ width: auto;
+ transition: all 0.2s
+}
+/* Project Sections */
+.projects {
+ display: flex;
+ flex-direction: column;
+ justify-content: center;
+ align-items: center;
+}
+
+project {
⬇️ Suggested change
-project {
+.project {
You've forgotten the dot to indicate it's a class selector.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +.project__image-image {
+ width: 85%;
+ position: relative;
+}
+.project__image-image img {
+ width: 100%;
+}
Combine those two, remove the extra wrapping
and call the class .project__image. The .project__image-image img is a
bit overcomplicated.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + }
+}
+
***@***.*** moveInRight {
+ 0% {
+ opacity: 0;
+ transform: translateX(10rem)
+ }
+
+ 100% {
+ opacity: 1;
+ transform: translateX(0)
+ }
+}
+
***@***.*** moveInBottom {
This and fadeIn are not used. Have a look at what else might be excess.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +
+ 100% {
+ transform: translateY(0)
+ }
+}
+
***@***.*** fadeIn {
+ 0% {
+ opacity: 0
+ }
+
+ 100% {
+ opacity: 1
+ }
+}
+.socials__github,.socials__email,.socials__linkedin {
YOu could use one class for all those, called .socials__icon and then
wrap them in a div with flex box where you use the gap property
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +.contact_text{
+ font-size: 1.8rem;
+ margin-bottom: 6rem
+}
This overwrites the definition from a few lines up. I guess you can remove
the one up there and leave this.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> + height: 4rem;
+ width: 100%;
+ background-color: var(--drk-blu);
+ color: #fff;
+ font-size: 2rem;
+ font-weight: 700;
+ cursor: pointer;
+ transition: all 0.2s;
+ text-transform: uppercase;
+}
+.contact__form-submit:hover {
+ transform: scale(1.1);
+ font-weight: 700
+}
+.contact input, .contact textarea {
+ width: 500px;
This will cause you horizontal scroller on devices below 500px width
unless you change it with a media query.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +.contact input, .contact textarea {
+ width: 500px;
+ resize: none;
+ border: none;
+ font-size: 1.8rem;
+ color: inherit;
+ background-color:#a1d1c8 ;
+ font-size: 1.6rem;
+ font-family: 'Courier New', Courier, monospace;
+ border-left: 2px solid transparent;
+ border-radius: 2px;
+}
+.contact input:focus,.contact textarea:focus {
+ outline: none
+}
+.contact input::placeholder,.contact textarea::placeholder {
Relying only on placeholder text is not good for web accessibility. It's
preferred to use a tag and to associate it to the input fields by using the
for attribute.
------------------------------
In html-css/week1/Portfolio/css/styles.css
<#14 (comment)>
:
> +}
+.about__content{
+ display: flex;
+ justify-content: space-between;
+ width: 100%;
+}
+
+.main-header{
+ background-color: transparent;
+ color: #fff;
+ padding: 10px;
+ text-align: center;
+ display: flex;
+ justify-content: space-between;
+ top: 0;
+ cursor: pointer;
I'd delete this, as it's causing confusion. pointer is used for clickable
elements, but here it's on the entire navigation. It misleads the user that
they can click the menu items from the padding space, but actually it works
only when you click exactly on the text.
—
Reply to this email directly, view it on GitHub
<#14 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BIMTRGREBL6H2PGZSRCZHYD2LKEIHAVCNFSM6AAAAABVJZJ32CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNRQGQ3DCMBQHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Hi, pleas ignore portfolio commit, i am working on the changes and suggestions from above comments , will be done by Friday |
jason-vasilev
approved these changes
Jan 25, 2025
There was a problem hiding this comment.
The page looks really good.
Just try to use more class selectors and don't copy everything from Figma. Be a bit critical of what it provides you as CSS. Some properties are either too fixed to the design there, and won't apply to minor content changes, or just are unnecessary.
Keep up the good work.
| height: 44px; | ||
| border: 1px solid #939393; | ||
| background-color: #f3f1f2; | ||
| font-weight: 400; |
There was a problem hiding this comment.
I think this might be already the default font-weight for input fields.
Comment on lines
+17
to
+18
| height: 68px; | ||
| width: 326px; |
There was a problem hiding this comment.
Those values are very strict. If you suddenly add more text to the button it might not work. It's better to let it grow accordingly to the amount of copy it contains. You can best do this by removing strict width and height and instead add padding.
| @@ -0,0 +1,261 @@ | |||
| @import url('https://fonts.googleapis.com/css2?family=Montserrat:ital,wght@0,100..900;1,100..900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap'); | |||
There was a problem hiding this comment.
You add many variations in font-style and weight, but probably don't use all of them. This will slow down the load speed of your page and ranking in Google. See if you can remove any of this.
| h2 { | ||
| font-size: 24px; | ||
| font-weight: 700; | ||
| line-height: 27.6px; |
There was a problem hiding this comment.
Avoid fractions of a pixel, as browsers interpret them differently and your website might look inconsistent between them.
| } | ||
| .header p { | ||
| color: white; | ||
| font-family:"Montserrat", serif; ; |
There was a problem hiding this comment.
Suggested change
| font-family:"Montserrat", serif; ; | |
| font-family:"Montserrat", serif; |
Comment on lines
+24
to
+27
| <div class="totalContainer"> | ||
| <p>Total</p> | ||
| <p>$333 / month</p> | ||
| </div> |
There was a problem hiding this comment.
Those are definitely not separate paragraphs, considering how short the copy is. I think it could be better marked like this:
Suggested change
| <div class="totalContainer"> | |
| <p>Total</p> | |
| <p>$333 / month</p> | |
| </div> | |
| <p class="totalContainer"> | |
| <span>Total</span> | |
| <span>$333 / month</span> | |
| </p> |
| <p>Total</p> | ||
| <p>$333 / month</p> | ||
| </div> | ||
| <hr /> |
There was a problem hiding this comment.
As commented in the CSS, this <hr> could be border-top: 2px solid white; on .bigTotalContainer.
|
|
||
|
|
||
| <button type="submit">Buy Now</button> | ||
| <p class="gradientText">Need help ?</p> |
There was a problem hiding this comment.
This is most probably supposed to be a <a>. What do you think?
|
|
||
|
|
||
|
|
||
| <button type="submit">Buy Now</button> |
There was a problem hiding this comment.
Add a class name. Eg. .primary-button.
Comment on lines
+41
to
+44
| <div class="visaCards paymentWrapper" tabindex="0"> | ||
| <img src="./Images/cards.png" alt="visa cards" /> | ||
| <p>Credit/Debit</p> | ||
| </div> |
There was a problem hiding this comment.
You've used tabindex in the effort to make this keyboard accessible. The best way to handle it would have been to make the <div> a <button>, and remove the <p> tag, as <button> anyways assumes text inside.
Suggested change
| <div class="visaCards paymentWrapper" tabindex="0"> | |
| <img src="./Images/cards.png" alt="visa cards" /> | |
| <p>Credit/Debit</p> | |
| </div> | |
| <button type="button" class="visaCards paymentWrapper"> | |
| <img src="./Images/cards.png" alt="visa cards" /> | |
| Credit/Debit | |
| </button> |
There was a problem hiding this comment.
Another approach would be to use a bit of ugly CSS selectors with <input type="radio> - https://codepen.io/menian/pen/raBZZmZ
|
Wow, such a detailed comments and corrections, noted and will be implementing these recommeded techniques :) |
Git week1/exercise 2/neepun
Git week1/exercise 1/neepun
Git week1/exercise 3/neepun
adding favfood
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
initial pull request