@geomydas
Posted
Hi @Ismael, your solution looks really good. I have finished reading all of your code and it seems good to me but it has few issues
My Feedback and Suggestions
- Have all of your frontendmentor projects inside a separate github repository instead of having 1 main repository with all of your projects there. Separating them by repository makes it less messier aswell and git push will be faster too
- Use a proper CSS reset. I reccomend using Josh Comeau's or Andy Bell's CSS reset as most people use them. I dont really reccomend making them from scratch as there is no need to reinvent the wheel and it may likely have mistakes too
- Use only one link to your CSS in your html. This is for bandwidth and performance, if it is too messy to have your all of you CSS in one file, you can use @imports and then use a build tool to inline them or you can alternatively use a CSS preprocessor such as Sass and use their partials feature
- Never ever set font related properties in px. This is extremely bad for accesibility and you should use the rem unit instead. See why
- Don't set a font size in the :root. Just apply them on the body and let all of the elements inherit it because CSS cascades
- Avoid setting text colors inside the body selector. You will just like override it anyways, I'm not saying it is bad but I just dont reccomend it
- Take a look at the BEM naming methodology and see it if helps you write better classes in HTML, more maintanable CSS, and write classes faster. It's not for everyone though as it is very verbose but try it and you won't go back
- Never use ids for styling. Use classses instead. Even though its a unique element, it has a very high specificity which makes it harder to override it later on. Ids are typically used for links and aria
- Replace the div with the class of links with a
<ul>tag as it is multiple links and wrap the links inside a<li>tag - Use a header or subheader as a replacement with the element that has the word "Ismael" on it. It's the largest text here and its the main topic here aswell
- Replace the em units with rem. Using em is fine but there is no point for it if it does not rely on another element with a font size. You would typically use em for spans inside a element that needs to be relative with the parents font size. Take the example below
HTML
<p><span>The</span> quick brown fox </p>
CSS
p { font-size: 1.25rem }
span { font-size: 2em}
It means that the span will be double the font size of its parent element which is the paragraph tag. You can use em too for margins for headers or paragraphs 12. Use mostly px for border-radius. This makes it so that that border radius stays the same even if the element shrinks. Using percentages is ok but it will shrink and may cause ellipses. You would use percentages if you want to make a element circular or stuff like that. 13. Use numbers for setting font-weight just to be consistent. If you are using numbers, go with fully numbers. If you are using words, go with fully words. Example is font-weight: 700, just make it consistent with your css
That's pretty much it. Don't be overwhelmed as in the later projects, you will become better with more feedback. Treat this as a to-do list and fix issue one by one. Have fun coding and keep on learning!
Marked as helpful
@ismeh
Posted
Hi @geomydas, thanks for taking the time to write such a detailed feedback.
I would like to discuss a few points.
- I think I'll continue with the big repo instead of the specifics (only for the repo dedicated to this website), you are right that it is a bit messy (I should share the link to the specific project instead of the global repo , my bad) but I think it is easier to find, review and organize all my projects that way. With regard to push speed, do you have any references? I don't really think the push performance will be worse, even if it becomes, it will be insignificant. I just found 1 discussion about this topic on reddit and maybe it gets slower at +100K commits and that won't happen on this repo.
- Regarding the CSS Reset, I didn't make my reset from scratch I just analyzed most famous resets. After some documentation, my final decision is to use a new mix(I'll show it on 4th project) I made after understanding some of this resets or just go with the CSS normalizer, at the moment I'll stick to the reset.
- It's true that adding more files it's worse for performance because it needs more request. At the moment it is easier to have the reset and the css for the project in two files to have it more organized, start new projects faster and ¿easier to understand(maybe? haha)? , but I will investigate 'imports' and CSS Preprocessors.
- I take note!
- 👍 I bring more references
- Then how should I approach to have default general color? 7.👌 I'll read about it.
- 👍 Thx, I hadn't thought of that.
- At the begging I did it but I changed my mind, I don't know why xD. Do you have any reason? Maybe just because it's a LIST of buttons? Haha.
- After trying I had to change the css selector to change <li> and also had to modify <ul> class, you can see the changes on last commit.
- 👍
- 👍
Again thanks for this big feedback!
@geomydas
Posted
@ismeh Those aren't buttons but links. Even thought it looks like a button or styled like a button, it is not a button. A button triggers an action such as posting a comment or toggling a menu, a link takes you into another page which is the case here.
The reason its a list is because that is is multiple links, they are related, and they are grouped
For having github repos for each project, I think its really just a matter of preference
For your CSS reset, I don't see the need for setting a min-width: 0 in the global selector, it seems useless to me. You can remove the margin: 0 in the body as you already used the global selector to remove all the margins on all elements.
The reference you said in 5. is very wrong, its very inaccesible also and may cause issues with third party code such as frameworks like Tailwind or Open Props. See why
Marked as helpful
