Skip to content
OverviewSolutions
3
Comments
3
Pablo Hernan Obrach
@pablo-obrach

All comments

  • Pablo Hernan Obrach•120
    @pablo-obrachPosted over 1 year ago

    Congrats on finishing the challenge

    The HTML looks good, but should be better to have the script in a different file insted of inside the HTML because if you need to add more scripts will be a madness to do it inside the HTML.

    The layout looks good but not exactly like the image provided by frontend Mentor, a good way to improve it s to follow the most accurate way the image given to you (in a real job that image it's made by the UX/UI desinger and it's approved by the client beforehand).

    The script logic it s fine but there s a BIG problem with the variables. You should never use var to asing a variable . That s because all var variables are at a global scope no matter where you used them and that can be an issue in a bigger script. Always use let or const at your script. Also the function onclick it s not a very well usage for the functions at the script . It s way better to use an eventListener() for the buttons or stuff you want to add a function.

    The layout for both screen sizes looks really good, at the desktop view you forgot to add the hover effect for the button before it will send the email wich also has a box-shadow property. Also if you check the images at the desing folder , in the active desktop image you can see that when the email it s not valid the placeholder should be transparent red and the border for the input also should be red as well as the invalid message.

    At the valid modal there 's a writting mistake here "Please open it and click the button inside to confirmyour subscription" confirm your subscription should be spaced. Lol that s a common mistake and nothig to worry about .

    Hope my feedback was useful for you, have nice day!

    Marked as helpful
    0
  • Pablo Hernan Obrach•120
    @pablo-obrachPosted over 1 year ago

    Congrats on finishing the challenge!

    *Everything looks pretty cool , but the desktop view doen't match like the one given to you

    from Frontend Mentor challenge, it s a difficult challenge but you did great.*

    Desktop view

    -The background from the header it s not like the one on the original version. Check if the image is at the assets folder. Also the header needs more space from the top , you can achieved that adding a margin-top at least of 2rem. The nav-bar should be next to de Logo instead of in the middle . If you are using flex at the header you can put the logo and the nav-bar into a single div and then another div for the sign-in/log-ing so you can put them into a container into the header and apply flex to that container with the property justify-content: space-between. That could fix that

    -The font should look like the ones given you by frontend mentor desktop view. You can find that info in the style-readme file . The colors for the fonts should also match with the color at the style-readme file.

    -To solve the image position you can use it as a background-image for the div and position it at the right side also if you have overflow issues with a property called overflow-x: hidden you could fix that and place the image next to the texts. (those texts shouldn't be a lorem , you have the text content at the files).

    The main issue i can see from your solution it s you didn't use the style-readme file and some images at the assets folder. But as i said before you did a great job and looks pretty cool.

    -The JS menu works as intended so there s no opinion about that.

    Hope my feedback helped you

    Marked as helpful
    1
  • Pablo Hernan Obrach•120
    @pablo-obrachPosted over 1 year ago

    Good job for finishing the challenge

    A few things could be fixed so it looks like the original desing

    Mobile view

    -The nav menu doesn't have the transparent grey background: that can be fixed adding a CSS propery to the body of the menu for example: bow-shadow: 0 0 0 100vmax rgba(0, 0, 0, 0.3). If for some reason you have an overflow from it you can use overflow-y:hidden;

    -Inside the Nav Menu the links font sizes shoud be bigger with less margin.

    Desktop

    -The header needs more margin or spacing from the main content.

    -The hover effects in the nav bar and the button are ok , but you forget to add the hover effect to the titles on the news section and the titles for the related content section too. That 's an easy fix.

    Anyway the site looks pretty good

    Marked as helpful
    0