Skip to content
OverviewSolutions
33
Comments
18
Mijail Hernandez
@MishaHernandez

All comments

  • Nofar Aviv•120
    @nofaravivSubmitted over 3 years ago

    I want to get better on the front-end development. Any comments or suggestions that help me improve are greatly appreciated.

    Stats preview card component

    1
    Mijail Hernandez•950
    @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 removing margin-top and using flexbox or grid 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
  • TrakaMeitene•505
    @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.

    Article preview with JS

    1
    Mijail Hernandez•950
    @MishaHernandezPosted over 3 years ago

    Hello TrakaMeitene 👋

    1. 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.

    2. 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 or let instead of var.
    • 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
  • Mijail Hernandez•950
    @MishaHernandezPosted over 3 years ago

    Hi 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
  • TrakaMeitene•505
    @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.

    Positioning elements

    1
    Mijail Hernandez•950
    @MishaHernandezPosted over 3 years ago

    Hello TrakaMeitene. Looks great, good job. 👋

    I would recommend that in your * {...} formatting rules you always use box-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
  • Lawrence•240
    @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!

    Huddle landing page (newbie) - my first attempt on Frontend Mentor

    2
    Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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 a padding 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
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Te ha quedado muy bien, sólo añadiría a la imágen de encabezado .card__head img un width: 100% para que no desborde su contenedor. Greetings and continue with the challenges.

    1
  • Kalvin Hart•140
    @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:

    1. 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?

    2. 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.

    3. 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!

    Insure landing page with CSS Grid and Flexbox

    2
    Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hello, 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 to position: relative.

    Greetings and I hope my feedback has been useful to you :)

    0
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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
  • Mijail Hernandez•950
    @MishaHernandezPosted almost 4 years ago

    Hi 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 a background in body.
    • 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