I am very new to frontend development and this is actually my very first complete frontend project. I'm curious as to whether I approached the task in the right way, that is, the way I structured my HTML code or if there was a better way I could have done it. I am also wondering if there's a way to write less CSS and achieve my desired results. I want to learn to be as efficient as possible and avoid redundancy. Being my first project I would appreciate any feedback/criticism as it would help me learn more and improve my work. Thank you in advance!
Zhiul
@ZhiulAll comments
- @crawler990Submitted almost 2 years ago@ZhiulPosted almost 2 years ago
Hi! Really great, amazing work :) There are still a few things you could do in order to make it better:
-
You should make it responsive all the way from mobile to desktop. I think desktop could start from around 768p. (You should use other media queries).
-
Let's say you set the desktop version to trigger at 768p. Either way, you must aim to make it look good. So you would need to set up some padding and other things. And when it comes to the hero image, chances are a
background-image
could fit better, as you could set it to automatically fit the container. -
You must use more semantic elements like
<header
><main>
<section>
and so on. Not only that, I highly suggest you also to give more meaningfull classes name, likehero
btn-primary
article
and so on, if you inspect high quality code you will see how some people name things really well. Also giving a try to the BEM convention can be great. -
In your
<header>
you woud like to use a<nav>
and when it comes to the links like Home, news and so on, you should use aul
inside the nav, useli
elements and inside those li elements include anchor tags. In this way, you would make it a lot more semantic. -
READ MORE
should be a button. Also try to use HTML headings when they would be semantic... -
I think you are using a small
font-size
for certain elements in comparison to the design. So I suggest you to make some texts a bit bigger. -
At last, but not least if you used rem you could make it more accessible. By using rem you could have some challenges, but, you would increase A11Y a lot.
Marked as helpful1 -
- @natalie-0073Submitted almost 2 years ago
Hi! Easiest project ever, used axios to handle api call. Looking forward to feedback! :)
@ZhiulPosted almost 2 years agoHi! It is looking great. There are some quick fixes or things that you could improve :)
-
You could use rem to make it more accessible.
-
When it comes to your
.advice-content__button
, you could create it with a<button>
or at least userole="button"
. -
When it comes to the transitions, try to narrow the transitions only to what is needed to transition, sometimes using all could be ok, but, here some unnecesary transitions could happen (e.g: when resizing the browser and so on, sometimes they can look strange, so I suggest you to be more aware of transitioning only what is needed)
Marked as helpful0 -
- @natalie-0073Submitted almost 2 years ago
Hi guys. Finally finished this project. Still have no idea how to make border of popup project green when selected. Tried every method I could find online but nothing worked. Looking forward to your suggestions.
@ZhiulPosted almost 2 years agoHi! Your project looks really great and your code looks is mostly very clean. 😃💫 Here's some feedback:
1- You could use
<a>
elements inside your.navbar-list__element
. It could be a bit more semantic (they most likely could be links), also it would make them focusable (also, you could consider to style how certain elements should be while in focus, like buttons and so on, sometimes you will want to set a tabindex).2- Overall your code looks really great. I suggest you to use
Prettier
or some kind of tool that could format the code automatically.3- When it comes to your JavaScript code, it is great, but I think you could make it a bit cleaner by doing a few things like naming the functions, so they could be like "toggleOverlay", "toggleHamburger" and so on.
4- You are using
innerHTML
it can be ok, but usually for setting text, if you're only setting texttextContent
seems to be a better practice.5- You should assign a type for each button. In this case, I think
type="button
should suffice. Also when it comes to some elements that are button but use other element, you should considerate to use a<button>
element or at least using arole="button"
.6- You should use
rem
a bit more in order to make the page more accesible. Some parapgrahs and elements that should change stay with a static font-size. Also, you could want to set thefont-size
in your.page
in rem, so you could use 1rem. More than that, if you're using rem in some text elements, you would most likely also need to handle some paddings and properties either by using rem or em (em can be more convenient in some cases); you should consider too some edge cases like if you increase browser font-size and the viewport is small, there could be some overflow.7- When doing transitions, only transition what is necessary. In this way, you could avoid some unnecesary transitions, and overall it is a better practice.
8- You could achieve making the border green in this way:
- You use a querySelector to check if some previous project has a class of green border (or selected, though this could also be written with a data attribute just like data-selected="true").
- If some previous
.popup-project
has a class/data-attribute of green/selected remove it. - Add the class of green/selected to the
.popup-project
that has just been selected.
And when it comes to getting the parent, you could use
closest()
😃 So it could be a function that runs when you click a.popup-project__radio
.It could also be possible to use has:(), however you must take into account that right now it only has a 82% support, so you should stick to JS by now and if you decide to use it, you should at least use a fallback. https://caniuse.com/css-has
Marked as helpful0