Skip to content
OverviewSolutions
14
Comments
17
Marcus Rangel
@WebDevCampos

All comments

  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! Great work! I've noticed that the buttons don't have hover states like the challenge asks. Besides that it's awesome!

    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! You did great! I just have a few observations to make if you allow me. Firstly, you need to wrap How did we do in a h1 tag, so the accessibility report won't find any errors. You can adjust it's font size and weight via CSS. Secondly, you could apply an addEventListener to the form itself, and iterate through all radio inputs, and apply a .forEach() method, for example. Your solution has some of these suggestions implemented, but what I mean is, you can have even less code, because there are a few things that aren't needed. If you fell like, you can take a peek at my solution : ) Please , feel free to reach me out and maybe we can work together on something! Kind regards!

    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! If I may, there's an observation I would like to make. Firstly, the component needs adjustment in it's responsivity, because it's stretching a lot haha. In my phone, for instance, it's width barely reach 200px. Secondly, you could wrap your index.html code in a main tag, for the sake of accessibility. And finally, the link provided for your code solution leads to the qr component solution, not this. I hope I could be helpful. Please, fell free to reach me out! Kind regards!

    Marked as helpful
    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! You did good, and if you allow me, I have some suggestions to make. Firstly, wrap all your index.html code, in a main tag. Secondly, you do not have to create a function specifically for one button at a time, you can select them all through document.querySelectorAll() and apply .forEach() method, for example. You can take a peek at my solution : ) These are the first steps. Please fell free to reach me out and we can work together! Kind regards!

    Marked as helpful
    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Nicely done man! If you allow me, I have a few observations, though. First, wrap all your code in a main tag. Second, the image should have a text inside the alt attribute, since it's the component's core. Great work, keep coding! If I could help let me know, and if you have any questions or doubts, let's work together! Kind regards!

    Marked as helpful
    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Nice job man! If you allow me, I have some suggestions to make: First, the img tag must have an alt attribute, mainly in this case, for the image is the product. Second, a small adjustment in border-radius, so it gets closer to style-guide parameters , and finally wrap your code in a main tag. If I could help, let me know! Kind regards!

    Marked as helpful
    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! You have an amazing work! This very project was the most challenging for me so far and I noticed you have some issues that I've faced myself. See, in android devices the maxlength attribute doesn't work like in PCs. So I've found a pen that fixed that, by K-ro. Check it out and let me know if it helped you! Kind regards!! https://codepen.io/K-ro/pen/NPWBNO

    Marked as helpful
    2
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! Congratulations on posting your first challenge! You did great, but if you allow me, here are a few suggestions: Firstly, always look at style-guide.md file that comes with the project. There you'll see the proper colors and sizes, for typography and for the elements. So you can see that you have to fix the Improve your front-end skills by building projects color. Actually it leads me to another topic: all projects must have a h1 tag, so this very text is a h1. If you feel that the default font-size for this element is bigger than you like, than just set your own size. Here is a tip to center your main element: body { background-color: hsl(212, 45%, 89%); font-family: Outfit; display: flex; justify-content: center; align-items: center; height: 100vh; } Doing this, you won't need to set a margin that big for your component and you won't need to set a heightor overflow either. These are the frist steps to improve your solution. If you feel like, you can reach me out and we can work together! Kind regards!

    Marked as helpful
    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hello there! You did great! For this project I have put lots of divs, but you could handle it with less code than me. Although, if you allow me, I have a few observations. First, when in mobile, the card has something that is causing it not to proper fit, some kind of overflow. I get this a lot when using padding or margin where is not needed, maybe you could review it. Secondly, there is sort of a horizontal rule between the image and the text, and at last, the word Companies has a typo, it happens a lot haha 😅. So, my advice is to use fixed widths in this project. You can set a max-width:1440px to the card itself, when in desktop and max-width:375px when mobile. I didn't check the code in deep, but I think you're fine, and have just these few adjustments. If you feel like, please reach out! Kind regards!

    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hello there! Yes, this is a tricky one challenge. If you allow me, here are a few suggetsions: For the image troubble, you may look for picture tag in HTML 5.

    There are other issues regarding responsiveness. First of all, you would want to set the following styles in your body tag:

    body {
    background: var(--main-background);
    display: flex;
    flex-flow: column;
    justify-content: start;
    align-content: center;
    min-height: 100vh;
}

    It will allow you to set the main tag a justify-content:space-between, and the two sections of the card will be equally apart.

    For the main tag it self, you can set

    main {
    display: flex;
    justify-content: space-between;
    max-width: 820px;
    background: var(--card-background);    
    margin: 7rem auto;
}

    The max-width attribute will avoid the stretch effect. For .main-content and .img-background, you'd want to set a width:100%, for the .main-content a padding:2rem is enough and finally for the order of element's to swap, you may want to set

    @media screen and (max-width:820px){
main {
    display: flex;
    flex-direction: column;
      width: 375px;    
}
.main-content {
    padding: 2.5rem;
    text-align: center;
    order: 2;
}
}

    If you have further questions, please, reach me out, I'll be more than happy to contribute somehow. I must say that looking at your solution now, mine might be over elaborated, I'll review my code right now haha 😅.

    Marked as helpful
    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! You did well, but if I may, there are a few corrections to make. Firstly, the background-image is repeating, you can fix that setting css background-repeat:no-repeat. Secondly, please review the component's width and height. You don't have to set a relative unit such as percentage, you can set a fixed size. Read the style-guide.md file and you'll see all the requisites for the project. The size for mobile devices for instance, it's 375px and for desktops it's 1440px, maximum. All projects must have an h1 tag, so you can replace the h3 and set the font-size till it gets close to final result. You can check the style-guide.md file to set the proper colors to the elements as well. Any questions or further assist, don't hesitate to reach me out! Kind regards!

    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hello there! Great work, smooth transition on icon menu! I have noticed an issue, however. If you open the side menu when screen is small, when you increase size screen again, the darken area remains. I guess that's the only thing I noticed, but you did great working with both grid and flexbox. Kind regards!

    Marked as helpful
    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi there! You nailed it, congrats! If you allow me, I noticed that you could do some small changes in order to organize your code. For instance, you can store the colors in css variables, so it gets easier to anyone who reads it, including yourself, in a later moment. Kind regards, great job!

    Marked as helpful
    2
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hello @XenerZ, how are you doing? I hope you are great!! So, you nailed the grid system, but if you allow me, there are a few corrections to make. Your project is not responsible yet, wich means, it's view is not optmized for different screen sizes. You should use those media queries in your code, which are already there, but to change the elements position and size, accordingly to the user's screen view. Also, I noticed that your styles are in located at style tag, inside index.html file. It is a good practice to separate your styles in a different stylesheet, and use a link tag to...well, link them haha 😅. So, these first two steps will improve your code immensely. Kind regards!

    Marked as helpful
    1
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hello man! First thing to consider, is to give all images an alt text. Secondly, about darkening the screen, I did like this: put a pseudo-element ::before on the side menu, wich is 100% wide, with a dark background-color. You can check it out here. Another thing, try to wrap your code around a main HTML element, to improve accessibility. Kind regards!

    Marked as helpful
    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi man! What an amazing job! I whish I did it haha! The tiny bitsy smallest thing, if you allow me an observation, is the second image, the one in 'Top 10 laptops of 2022', when in small screens it doesn't preserve it's size. If you set a max-width to the images from this part it will be perfect, I mean, perfect! Congrats! Kind regards!

    Marked as helpful
    0
  • Marcus Rangel•460
    @WebDevCamposPosted over 2 years ago

    Hi man! I just had a peek from your final result and it is very impressive! However, if you allow me, I have a few suggestions to point out. First thing, the paragraphs' colors in the "New" section are difficult to read, because of contrast. And I'm afraid the side menu isn't working. I really like your design and how you give the images a "cover when hover" object-fit thing. And the navigation items are really cool with the underline on hover. That's that. Just improve the contrast and check the side menu. Kind regards!

    Marked as helpful
    3