Skip to content
OverviewSolutions
6
Comments
18
Roberto Yukio Miyamoto Arita
@RyukioMiyamoto

All comments

  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Impecable work Andrii! I would just adjust the color on the <p> that's differing from the proposed design.

    As for the image, what I follow when solving the challenges is considering if the image would be something static or dynamic in a real project, maybe coming from the backend. For hero sections for example, I'd use the background solution as it won't likely change or have variations.

    For this QR Code challenge, in my conception, there could be another card with a different QR Code somewhere down the line, so I chose to make it a <img>.
    This is my personal take on this by the way, I have no idea if it's a common/good practice, I'll have to look it up later.

    Keep it up!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Hi Thato! Great job with the design, however I'm not sure if the font-family is the same as proposed by the design. As for the "insights", you could wrap it either in an <em> or <strong> tag and style it individually.

    Hope you find it helpful! Keep it up!

  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Hi Dan, great job. I looked into the why of display: block after my last comment and it seems the reason is that block elements take up all width available, while inline elements take only as much as they need to, which might make some whitespace visible.

    For the border-radius, my suggestion would be to give it to the container rather than individually (altough in some situations this solution may be better), and while that initially won't do the trick, adding overflown: hidden will solve it.

    Hope you find that helpful, have a great week!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Andrii you did a great job with the design! However I find it very hard to give feedback on the minified code. I'd like to point a few things anyway:

    • The rating are not navigable through keyboard, which is an accessibility issue. An improvement on this would be to use <button> or <input> instead of <div>
    • Your script is not preventing the user to proceed without a rating, which gives an "You selected a undefined out of 5" on submit
    • Mobile ( below width +-400px ) is not applying the style to the selected rating, although it's still functional

    Hope I could be of help! Keep it up!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Hey Andrei, good job! To fix the hover overflowing, set the img to display: block to get rid of the whitespace.

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Excellent work Dan! I had the same issue regarding the img height, while I solved it differently, there's a simple solution that is to add display: block to it, erasing the white space. I'm not 100% sure about why but I think the reason it works is because it makes the element take up all available space in its "line". Anyway, good job!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    I would just recommend you change the rating from divs to buttons or inputs, as they're not currently selectable, which makes them inaccessible to keyboard users. There's also some inaccuracy regarding the design, like the circle sizes and border-radius on desktop, but nonetheless, excellent work!

  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Great job lukasz! I would just like to make two suggestions:

    • Final result is not matching 100% the proposed design, like font-size on the heading.
    • Although it was a very nice addition to prevent user from submiting without a rating, there's more elegant ways to do so than using alert(), like adding an <p> that only shows on submit, for example.

    There's also the matter of accessibility issues, which frontend mentor is pointing in your report:

    • Document should have one main landmark
    • All page content should be contained by landmarks

    Basically you can solve both by just changing your wrapper div to a <main> tag, which is of significant importance to screen readers. Hope you find this helpful! Good coding.

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Great job overall Majdi! Regarding the overlay, try using mix-blend-mode: multiply on a separate <div> instead of a gradient, it worked for me (even though I couldn't get the exact tone as well). Hope I could be of help, if I wasn't clear enough take a look at my solution. Also, the mobile version is missing, are you still working on it?

    Good coding!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    That was a great addition, good job. I would just suggest you consider the prefers-reduced-motion query, regarding the animations.

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Good job Rowen, I would just suggest adding "object-fit: cover" to the <img>, to avoid deforming it. Also, in the mobile you forgot to adjust the individual border-radius, so it's not matching the design. My suggestion is to add a border-radius to the container itself rather than the img.

  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    This is stellar work! I've got nothing to say regarding the styling since you clearly got that covered, but I do have a suggestion regarding accessibility, as some people get motion sickness from animations, and someone once pointed out to me the prefers-reduced-motion query, which I find very important and always try to implement ever since (sorry if you're already aware of it).

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Great job overall, some improvements I would suggest:

    • Adding a transition to the hovered effects
    • Set the proper height in the img to match the hover effect background, as it's overflowing

    Good coding

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Great work overall, but I would like to comment on the button's hovered state, as it has a border property, while the regular button doesn't, which is causing your design to "jump" on hover, even though it's just 1px.
    An easy fix is to add the border to the btn class, rather than just btn:hover. Your html is also missing a <main> tag, which is an accessibility issue. Just wrap you div.container in it and you should be good.

    Hope you find it helpful!
    Good coding.

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    @RJC26 I use Figma for this purpose, it's not optimal but you can open the design files and measure the spacings with it. I tried using VisBug too, which is a chrome extension, but I had a hard time using it, but it's worth a shot

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    You just missed a few details like the shadows, and the background image could cover everything by using background-image: contain, but you did a great job overall! As for the extensions, I would recommend using Lighthouse, for checking accessibility, SEO, perfomance and more, for both desktop and mobile.

    Good coding!

    Marked as helpful
  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    This is a proper responsive design, which means it's gonna adapt to the navigator rather than wait for it to "break", which would be an adaptative design, using a query. The only improvement I would suggest you would be to center the container, which could be done by giving it a width and a margin: 0 auto (that's how I would approach it, but I'm sure there's better ways). Anyways, great work! Good coding.

  • Roberto Yukio Miyamoto Arita•320
    @RyukioMiyamoto

    Great work! I just see some room for improvements regarding the buttons:

    • Font is not applying, what I found was just a typo:
    line 126 font-family: "Lexa Deca";
    • You could make your code more DRY by applying the hover effect on the btn class instead of individually. It would look like this:
    .btn:hover {
  background-color: transparent;
  color: var(--color-transparent-white);
}

    Also, I would suggest you add the border to the btn class and just change the background color and color on hover, because, since the border wasn't there before, the button would "jump" (even though it's just 1 pixel).

    Hope you find this helpful! Good coding.

    Marked as helpful