Your feedback and suggestions are greatly appreciated!
Exequiel Arco
@quielLovesLasagnaAll comments
- @Charles-Mc-VigoSubmitted 10 months ago@quielLovesLasagnaPosted 10 months ago
Ayos! CHARLES MC VIGO
Here are some suggestions:
-
Use external CSS instead of internal, this makes your code more cleaner and organized.
-
In the case of your CSS reset:
*{ margin: 0; padding: 0; font-family: 'Inter', sans-serif; }
I suggest you remove the font-family declaration and add it inside the body. The reason for this is that it is more of a good practice to add the properties that affect the texts inside the body because it also gets inherited, and the use case for the universal selector, many devs suggest that you use it for removing default padding, margins, and setting the box-sizing to border-box. You can read more about it here and here
- I notice an error in this part:
main img{ height: 80px; width: 8-px; <------- border-radius: 50%; padding: 25px; cursor: pointer; }
-
I also noticed an inconsistency in your markup, you added an id for your location and quotes, I suggest (for styling's sake) you use a class and don't use it only for two elements, use it for elements that need it. Using a class for selecting elements to style is a good practice instead of adding ID randomly because you want to style them differently.
-
In this part:
<div class="links"> <a href="#">GitHub</a> <a href="#">Frontend Mentor</a> <a href="#">LinkedIn</a> <a href="#">Twitter</a> <a href="#">Instagram</a> </div>
This is a list of social media platforms, it's more semantic to use ul and li here. Like this:
<ul class="links"> <li><a href="#">GitHub</a></li> <li><a href="#">Frontend Mentor</a></li> <li><a href="#">LinkedIn</a></li> <li><a href="#">Twitter</a></li> <li><a href="#">Instagram</a></li> </ul>
That's all! I hope these may help you.
Marked as helpful0 -
- @TheTreeveloperSubmitted 10 months ago
Thanks to Marzuk Sanni for the advice. I was able to start building from the mobile version, then made it responsive for the desktop version. Any feedback is highly appreciated, thank you.
@quielLovesLasagnaPosted 10 months agoGreat job, Tolulope Oluwabukunmi! Here are some suggestions:
- I can see that you did:
<h1>Jessica Randall</h1> <h3>London, United Kingdom</h3>
Here are some things that you must keep in mind when using headings:
- You should only have 1 <h1> element in your document. (main heading/main topic)
- You should not have a secondary heading <h2> if you don't have a main heading <h1>
- You should not have <h3> if you don't have <h2>
- You should not have <h4> if you don't have <h3> and so on...
Changing your code from the code above to the code below is the correct way of marking it:
<h1>Jessica Randall</h1> <h2>London, United Kingdom</h2>
- I can also see that you did this:
<section class="button-container"> <button>GitHub</button> <button>Frontend Mentor</button> <button>LinkedIn</button> <button>Twitter</button> <button>Instagram</button> </section>
this part of the profile is a list of social media platforms. using a ul and li's is the correct way of marking it:
<ul class="button-container"> <li>GitHub</li> <li>Frontend Mentor</li> <li>LinkedIn</li> <li>Twitter</li> <li>Instagram</li> </ul>
- Inside your external CSS file, I can see that you did not reset your CSS although this is "optional" (?) most/every developer does this so I suggest you do it as well. You can copy my CSS reset if you want to:
*, *::before, *::after { margin: 0; padding: 0; box-sizing: border-box; } img, svg{ mix-width: 100%; display: block; }
You can read more about this here or look it up online.
That is all. I hope these suggestions may help you.
Marked as helpful0 - @lekkaaudisySubmitted 11 months ago@quielLovesLasagnaPosted 11 months ago
Nice work, @lekkaaudisy ! Here are some suggestions:
-
I can see that you are doing a great job at the markup but instead of using
div
tags for each box/card, I suggest you usearticle
tags. -
Try adding metadata to add additional information about the website or project when you host it on the internet, you can learn more about it here
-
I noticed that you are importing separate fonts inside your style.css, I suggest you select all of the fonts needed for the project and then import them in bulk (which is a common practice) instead of importing them separately.
That's all :)
0 -
- @MateusCamargoS-1Submitted 11 months ago
Project Challenge Reflection
This project, no matter how seemingly simple, presented me with a considerable challenge. I'm uncertain about what caused more trouble—whether it was the positioning or decisions related to the image size. Anyway, I would appreciate it if you could take a look and offer any tips you may have. Thank you all very much.
@quielLovesLasagnaPosted 11 months agoNice work @MateusCamargoS-1 ! Here are some suggestions:
- Instead of using
p
tags inside adiv
to make a list for the "statistics", I suggest usingul
tags andli
, this is a more semantic way of representing a list of stats.
- It's nice that you are using a
section
tag to contain your content but if we think about this, it's better to use thearticle
tag as it is best suited in this case... You can read more about thearticle
tag here
Marked as helpful1 - Instead of using
- @muhibkhan2005Submitted 11 months ago
I have completed this frontend mentor challenge please give some feedback
@quielLovesLasagnaPosted 11 months agoNice work @muhibkhan2005!
Your output is looking good but here are some suggestions to improve it:
- I see that you did:
<div class="container"> <div class="container-main"> <div class="container-img"> <img class="qrimg" src="./images/image-qr-code.png" alt="qrimg"> </div> <div class="title-heading"> Improve your front-end skills by building projects </div> <p class="title-para"> Scan the QR code to visit Frontend Mentor and take your coding skills to the next level </p> </div> </div>
Instead of using a
<div>
to contain your main content, use the<main>
element to give it semantic meaning,<div>
does not have a semantic meaning which is not accessible. You can read more about the main element here-
Setting a
min-height
for the body and not thediv
with theclass=" container
it's good that you are using CSSflex-box
to center your content but setting theheight
to the first container is not ideal (this is just a personal preference) but most developers set this property to the body when doing small projects like this, usingmin-height
for the body makes it more responsive especially in cases where your content is bigger than the actual container whileheight
specifies afixed
dimension to an element in general. -
Use the font and colors required for the project. You can see the instructions inside
STYLE-GUIDE.md
I think. -
You are using both internal and external CSS. You did not remove this part and transferred the CSS declaration in your external CSS.
I hope this will help you improve...
0 - @nicholasboyceSubmitted about 1 year ago
How exactly did you go about making sure the box shadow was as close as possible? How did you go about making sure your image was sized correctly? Did you put the image dimensions in the HTML img tag?
@quielLovesLasagnaPosted about 1 year agoHi, Nicholas. Nice question! This was the first challenge that I took, to answer your questions:
How exactly did you go about making sure the box shadow was as close as possible?
- Using the box-shadow property, we can basically give it 5 values (x-offset, y-offset, blur, spread, and color). I checked your code and it seems you gave a value of -6px to your spread, in my case, this is unnecessary. What I did was:
box-shadow: 1rem 0 4rem rgba(0, 0, 0, 0.2);
I only gave it 4 values (x-offset, y-offset, blur, and color) which worked for me. You need to experiment with this to achieve your desired outcome.
You can check this out to learn more.
How did you go about making sure your image was sized correctly?
- I did not define a fixed size for my image, what I did was I added the
max-width
property to my card container and made the image occupy 100% of the width:
.container { max-width: 30.5rem; } .qr-img { max-width: 100%; }
Did you put the image dimensions in the HTML img tag?
- No, I prefer adding styles to my elements externally which is the proper way of adding styles.
0 - @quielLovesLasagnaSubmitted about 1 year ago
While implementing the hover state of the button, I encountered something that was unexpected: when the button is hovered, it should change its background to a gradient, I was successful in doing that but when I hover away from the button, it flickers, it would be great if someone could explain to me why that is...
@quielLovesLasagnaPosted about 1 year agoHey guys, I just found out that transition does not work for gradient background that's why my button flickers whenever I hover out. But I just found a quick and easy solution for this. Check out this link if ever you encounter this problem too.
Kind regards, Quiel
1