I want to get better on the front-end development. Any comments or suggestions that help me improve are greatly appreciated.
Mijail Hernandez
@MishaHernandezAll comments
- @nofaravivSubmitted over 3 years ago@MishaHernandezPosted over 3 years ago
Hello Nofar Aviv π
It looks great, congratulations.
I just have a little remark: in desktop layout the main component has a lot of
margin-top
and this can produce a scroll bar on some screen sizes. So I would recommend removingmargin-top
and usingflexbox
orgrid
to center it horizontally and vertically.body { display: flex; align-items: center; height: 100vh; } @media (min-witdh: 1200px) { container {margin-top: 0; } }
I hope this is helpful to you. Happy coding π
0 - @TrakaMeiteneSubmitted over 3 years ago
Script tag is in HTML, have no idea if it is the best practice, but it's so small, I figured that it don't matter. But I con not make the button another color on mobile. It pissed me off :D This project was a little challenge for me.
Fell free to give some feedback how I could do it better.
@MishaHernandezPosted over 3 years agoHello TrakaMeitene π
-
Adding javascript in the html is not a good practice, but if the script is very small then there is no problem and rather it turns out to be more efficient.
-
To change the button you just need to add to the script a constant that gets the button's
id
and then inside the condition where the button must change color add the styles. For example:
var x = document.getElementById ("pop"); const btnShare = document.getElementById ("btnShare") if (x.style.display === "none") { x.style.display = "block"; btnShare.style.backgroundColor = "hsl(217, 19%, 35%)"; btnShare.color = "white"; } else { x.style.display = "none"; btnShare.style.backgroundColor = "hsl(210, 46%, 95%)"; btnShare.color = ""; }
As additional observations:
- To initialize a variable use
const
orlet
instead ofvar
. - To add styles from javascript I recommend using
classList
which gives you more options. [HTML DOM classList Property] (https://www.w3schools.com/jsref/prop_element_classlist.asp)
I hope I can help you with these recommendations. Happy Coding β
1 -
- @TrakaMeiteneSubmitted over 3 years ago
First time project with grid container.
@MishaHernandezPosted over 3 years agoHi TrakaMeitene π
Good job! Looks very similar to the challenge design. There is only small detail, you should rewrite the margin to the body (
margin: 0
) because this can generate a vertical scroll bar on some desktop screens.Greetings and happy coding! π
1 - @TrakaMeiteneSubmitted over 3 years ago
A little tricky, but at least it looks good on mobile and tablets . Made some changes, solving design problems. Thank you for comments.
@MishaHernandezPosted over 3 years agoHello TrakaMeitene. Looks great, good job. π
I would recommend that in your
* {...}
formatting rules you always usebox-sizing: border-box
, so that margins and padding do not affect the size of your boxes.Always avoid using ID's, especially if your design doesn't use javascript. In similar elements, for example the three icons: file, folder and cloud, you could use pseudoclasses to select each one.
Sometimes the use of
clip-path
can be replaced by something simpler like a correctly positioned geometric figure. For example the.sturis
element could be a semi-hidden triangle in the pop-up message.The font family should be assigned in the
body
so that all elements inherit it and not have to repeat this property in several rules.Greetings, and continue with the challenges π
1 - @LawrencePryerSubmitted almost 4 years ago
I have been watching and following along with some HTML and CSS tutorials for the past month. This is my first effort trying to replicate a design on my own.
I'd appreciate any feedback and suggestions for improving the way I have written my code. Hopefully it is easy to follow and that I have not included any redundant lines of code.
Thank you in advance for your feedback!
@MishaHernandezPosted almost 4 years agoHi Lawrence,
- The layout looks good, but I recommend using the <header> tag only as a container for the logo, the <main> tag should not have a <header> tag as the parent container, and the social media buttons could go inside a <footer tag >. I recommend checking about the "html5 semantics".
0 - @GroottrooGSubmitted almost 4 years ago
I think this website will look as it is only in my Desktop . ;(
@MishaHernandezPosted almost 4 years agoHi Nizamuddin, I recommend:
- Use the patterns as background images and avoid some complications with the positioning.
- Check Flexbox and if possible also Grid, this will help you to improve the distribution of the elements within a container.
Greetings, check the documentation and continue with the challenges :)
1 - @Karimsamir112Submitted almost 4 years ago
Please feel free to give feedbacks
@MishaHernandezPosted almost 4 years agoHi Karim, I have some observations for you:
-
I recommend not using <br> tags to generate spacing between elements, instead use margins, padding or spacing via flexbox or grid.
-
Use <buttons> tags instead of <div> and use the
font-size
property instead of header tags. -
In the action call (
.start1
) use a <form> tag as a container for the text box and the button. Use apadding
in the main container and use margins instead of <br>. -
In
.social-media
you use fontawesome icons but you must assign font css properties to give it size and color.
Finally I recommend that you review documentation about semantic html. Greetings and continue with the challenges :)
2 -
- @gianicolajaraSubmitted almost 4 years ago
Cualquier consejo serΓ‘ bien recibido :)
@MishaHernandezPosted almost 4 years agoTe ha quedado muy bien, sΓ³lo aΓ±adirΓa a la imΓ‘gen de encabezado
.card__head img
unwidth: 100%
para que no desborde su contenedor. Greetings and continue with the challenges.1 - @kalvinhartSubmitted almost 4 years ago
This was my first attempt at developing something mobile first. It was also my first attempt at making a landing page.
There are several issues that I have come accross and would appreciate any help/advice:
-
Upon hosting the page with github pages, I seem to have lost 2 images - the patterns for the intro section that were in the ::before and ::after. They show up fine on my local machine during development, why are they not loading on the github page?
-
With screen widths < 375px the main image scales down which then leaves the ::before pattern image lingering and looking out of place, I am not sure how to fix this. It might be hard to see what I mean seeing as the image no longer displays as per point 1 above.
-
Again with the intro pattern images, this time the ::after image on large screens. It shows above all other elements despite my header element having a z-index of 100 and position: fixed. I am not sure why this is happening?
Any other tips/advice would be appreciated as I'm sure I probably haven't used the mobile first approach in the most efficient way?
Many thanks!
@MishaHernandezPosted almost 4 years agoHello, Kalvin Hart!
- The header element is shown below the pattern because its z-index does not work for positioning with static values (
position: static
), so I recommend changing it toposition: relative
.
Greetings and I hope my feedback has been useful to you :)
0 -
- @Enol-IgaretaSubmitted almost 4 years ago
My first code. I have been studying self-taught for 1 month. Try to make the best use of gridd and flexbox. I accept any help, advice and criticism. Thanks for your time :)
@MishaHernandezPosted almost 4 years agoHi Enol-Igareta!
- The problem is the left margin of the image (
.card__img
) inside the main container (.container
), this margin has a size that overflows the grid, and finally the margins that help to center this container also overflow it from the viewport. - A solution would be to remove the left margin and align the image using flexbox or assigning it
display: block; margin-left: auto
;
Greetings and I hope my comment has been useful :)
1 - The problem is the left margin of the image (
- @PeterklinkSubmitted almost 4 years ago
No Problems on this one. If you spot something let me know.
@MishaHernandezPosted almost 4 years agoHi Peter!
Your solution has a good media response and is visually fine. I just have a few observations:
- The password input should have the attribute:
type = "password"
. - Buttons should have a brighter active state.
- The tag for Terms and services should be an
<a>
with active status. - Some tags like
<a>
and<input>
need accessibility attributes.
Greetings and continue with the challenges π
1 - The password input should have the attribute:
- @BatoolHasanSubmitted almost 4 years ago
Would love to hear feedback on how to improve the code
@MishaHernandezPosted almost 4 years agoHi Batool H.
Visually I see it very well. I leave you some observations:
- The use of the
picture
tag is not very useful in this context, since the idea is not to group images but rather to use them as abackground
inbody
. - I recommend you center the
.card
container inside body using Flexbox. - You make good use of class names but it would be better if you adapt it to a methodology like BEM :)
Greetings and continue with the challenges!
1 - The use of the