Skip to content
OverviewSolutions
31
Comments
33

Andreas Remdt

@andreasremdtGermany950 points

Fast, secure and good looking websites? Yes please! That's what I do.

I’m currently learning...

Rust

Latest solutions

View more solutions

Latest comments

  • Andreas Remdt•950
    @andreasremdtPosted over 1 year ago

    PS: I just noticed that the preview screenshot is broken. That might be because I used the latest CSS features, such as color-mix and CSS Nesting which might not be supported yet by the image generator. If you encounter the same issue, you might have to enable experimental feature flags in your browser.

    0
  • Andreas Remdt•950
    @andreasremdtPosted almost 2 years ago

    Hey @ricardosilvasx,

    Congrats on solving the challenge, well done! Your solution looks good, I like that you wrapped everything in a main tag which is a good practice. Here are some additional suggestions:

    • Try and incorporate a heading. Every website needs at least one heading to explain the content. Always start with the highest level (h1) and then descend down when it makes sense. In this example, I would make the text Improve your front-end skills by building projects the main heading.
    • You don't need the div.qrcode, just apply the styles of it to the main element directly.
    • Your image doesn't have any width and height attributes, which I would advice you to set. They allow the browser to better calculate the image dimensions and hence optimize rendering, which results in a better loading experience.

    Keep up the good work and let me know if you have any questions!

    Marked as helpful
    0
  • Andreas Remdt•950
    @andreasremdtPosted almost 2 years ago

    Hey @MikeLee0358,

    Congrats on finishing the challenge! Let me try and answer your questions:

    To limit the width of the container, you can use max-width. I see that you used the clamp() function, which also works but is a little overkill for your needs. Setting max-width: 450px will prevent the container from growing any bigger but still stay flexible on smaller screen sizes.

    The image inside the container is already set to width: 100%, which is great. It won't grow any bigger than the container and will shrink with it. I'd suggest adding the width and height attributes in the HTML, so that the browser knows how big the image is. This improves the loading experience, as the space is being "reserved" until the image has loaded. Also, don't forget to set height: auto in your CSS to maintain the correct aspect ratio at all times.

    For this challenge, you don't need to change the font size for smaller devices if I remember correctly. So you're good already. Try to set the correct font family though, as this will improve the visuals a lot :-)

    Some other things I noticed:

    • I would choose an h1 for the heading, as it's the first and only one. Headings in HTML should start from the highest level and descend down if necessary. Since you don't have any other headings, h1 is the best choice here.
    • Try and use more semantic HTML tags. For example, you could replace the div#QRCode with a main#QRCode to mark the card as the main landmark on this page. Semantics are an important aspect of HTML development and make life easier for search engines, screen readers, and others. Have a look here for more information.

    By the way, the method you used to align and center everything (display: grid) is really clever, nice one!

    Let me know if you have any questions, keep it up!

    0
  • Andreas Remdt•950
    @andreasremdtPosted almost 2 years ago

    Hey Giorgian,

    Congrats on finishing this challenge, your solution looks good! I found a few issues that you might want to fix:

    • The interactive elements (input, buttons) don't have clear focus or hover states, which should definitely be added for a better user experience and accessibility.
    • The input and button are not within a form, which makes you lose some important functionality like pressing Enter to submit and send it. This also hurts accessibility.
    • The error appears once I try to submit with an invalid email, which is great. However, once I type in a correct email address and submit, the error stays there, which is confusing to users. A good practice would be to hide the error when the email is correct and the user leaves the input (blur), presses the button, or even while typing (input event). So, make sure that you add an else condition in your JavaScript for when the email is valid.
    • Using regular expressions for validation is a common practice, but I don't recommend using these for emails, as there are tons of variants out there, and hardly any of them get it right. Some are too strict, others are too lax, and most don't cover all edge cases. Browsers these days already offer APIs to validate input. I recommend you look into constraint validation, which makes it so much easier validating HTML forms.
    • The social icons at the bottom are wrapped inside a button. I think that's okay for this challenge, as there's no action behind these; I would still advice using an a element, as social links tend to redirect you to a different website, whereas buttons are more suitable for triggering interactive JavaScript.

    I hope my feedback helped you out, let me know if you have any questions. Keep up the great work!

    0
  • Andreas Remdt•950
    @andreasremdtPosted almost 2 years ago

    Hey @ricochet69!

    Congrats on solving this challenge, the result looks great - it's very close to the design!

    Your initial idea of using a main tag to wrap everything is good. To be accessible and semantically correct, every page needs a main landmark, which in this case is the card itself. However, I would alter your HTML structure slightly: you can remove the main.container and apply its styles to the body element. Then, you can change the div.card to main.card. This way, you save on unnecessary elements and make your code clearer while still being semantically correct.

    Your margin and padding look good to me, but you could make it even simpler if you applied the padding to div.card-details. Since both the heading and paragraph are direct children, they would be moved into position by that change. I am not sure if you even need this padding, though, might be unnecessary. The margin on both elements looks good.

    I am not sure why you went with height: calc(100vh - 1px) on the container? You should be able to use min-height: 100vh without the need for calculations.

    Let me know if you have any questions and keep up the good work!

    Marked as helpful
    0
  • Andreas Remdt•950
    @andreasremdtPosted almost 2 years ago

    Hey @JorgeAMendoza,

    Great job on this challenge! I played around with it and everything works as expected. I especially love the fact that you wrote good E2E test coverage and used ESLint and TypeScript. Your code is very organized and clear, the folder structure and naming are consistent and sensible. I also noticed your attention to accessibility, which is appreciated. So, very well done :)

    I found a couple of minor things you might want to fix, but these are only little details:

    • The theme and font choice are not remembered in the browser. If I refresh after switching to dark mode or another font, it's back to default. Persisting the user's choice in local storage would be a nice addition.
    • While the audio playback works fine, you could think about adding a "play" state, which disables the button and renders a different icon, like a spinner.
    • Some fonts are not set correctly. For example, if you select the serif font, the main heading is still set to monospace. Additionally, the font dropdown doesn't have the right font families.
    • Inside font-context.tsx, you are using 2 contexts. If you don't think that this is necessary, you can have the font value and setter function on the same context and avoid the overhead.
    • You wrapped the search input inside a label, but the label doesn't have any text content. While technically speaking this is not an error, you can omit the label and rely on placeholder and aria-label. It's usually an UX anti-pattern to not have a label, but since the design is this way there's nothing us developers can do...

    Not an issue, just a tip: you implemented your own solution for the font dropdown, which is really good. For the future, I highly recommend using a headless library such as Headless UI or Radix UI. These libraries make it incredibly easy to build such components, because in reality it's quite hard to cover all bases (accessibility, keyboard navigation, edge collision, etc.).

    Keep up the great work and let me know if you have any questions :)

    Marked as helpful
    1
View more comments