Skip to content
OverviewSolutions
12
Comments
5
P
Radek Tomasek
@radektomasek

All comments

  • Radek Tomasek•160
    @radektomasekPosted about 1 year ago

    Hello Yaroslav 👋,

    I had some time to have a look at the code and here are some observations/recommendations that you can apply:

    • You might consider using CSS properties to define your variables. If you look at your CSS code, you are duplicating some stuff like colors/fonts. You can reduce that repetition by declaring the properties and then applying them. More info here.

    • You are using absolute units in places where you might consider using relatives ones. A good example is font-size. If you use absolute units (pxs), you are putting some unnecessary barrier for people with accessibility needs. If they change the default font size in the browser settings (making fonts bigger), there won't be any change on your site because of absolute units. You need to use relative units like rem, em ideally. Here is a good article if you would like to learn more.

    Otherwise, well done with the implementation.

    Have a great day. Radek

    Marked as helpful
    1
  • Radek Tomasek•160
    @radektomasekPosted over 1 year ago

    Hello Eduardo 👋,

    I had some time in my morning and had a look at your solution. It looks great, well done. 👏

    Here are my few observations/recommendations for tweaks you can make to simplify your structure even more:

    Simplifying the Flex structure

    You have two containers informations and music-price both set as display:flexbox. My understanding is that you are using for to display the icon and the price container. It does the trick, but you can simplify it by having one flexbox container with three children.

    • The parent in your case will be the container .informations
    • The first child will be the logo (icon music).
    • The second child will be the flex (colunn) container with the anual price.
    • The third child will be the change button.

    The you can use flex-basis for each child to set an appropriate width. On top of that, you can also use gap for the parent, but in this case it's not necessary.

    Therefore, your code will look something like this (please be mindful of comments, these are just some non-standard HTML as I wasn't able to put classic ones for you to see):

    # Flex container
<div class="informations">
# Flex child 1
<img src="images/icon-music.svg" alt="icon music">
# Flex child 2
<div class="price">
<strong>Annual Plan</strong>
<span>$59.99/year</span>
</div>
# Flex child 3
<a href="#">Change</a>
</div>

    If you slightly update the CSS like this, you will get the same result as you have now (minus that removed element .music-price which we don't need). The CSS might look like this:

    main .informations {
display: flex;
align-items: center;
justify-content: space-between;
margin: 2rem 0;
background: hsl(225, 100%, 98%);
padding: 1.5rem 1rem;
width: 100%;
border-radius: 0.8rem;
}

main .informations .price {
display: flex;
flex-direction: column;
}

main .informations a {
color: hsl(245, 75%, 52%);
font-weight: 600;
font-size: 90%;
transition: all 0.3s;
}

main .informations img {
width: 3rem;
height: 3rem;
}

    If you go even further, you can remove the justify-content: space-between from the flex parent and this will put all the elements close to each other. Then you can add a gap: 1rem to your Flex parent, which makes things more spread. (Flex gap is a very useful property).

    The last thing is to extend the flex basis for the second child and this will make your whole thing look closer to the design. (I also added an extra CSS selector for aligning the text to the left)

    main .informations {
display: flex;
align-items: center;
margin: 2rem 0;
background: hsl(225, 100%, 98%);
padding: 1.5rem 1rem;
width: 100%;
border-radius: 0.8rem;
gap: 1rem;
}

main .informations .price {
display: flex;
flex-direction: column;
flex-basis: 8rem;
}

main .informations a {
color: hsl(245, 75%, 52%);
font-weight: 600;
font-size: 90%;
transition: all 0.3s;
}

main .informations img {
width: 3rem;
height: 3rem;
}

main .informations .price span {
text-align: left;
}

    I hope it makes a sense. It's a small tweak relatively, but it will simplify the structure quite a bit.

    general improvements tips

    • You should also consider using CSS variables for repeating elements (colors, fonts). More info here
    • You might consider using mobile first design. Start constructing your design from the smaller displays to larger ones. I think it might be easier to reason about it (but it's primarily a matter of preference). I feel for these challenges it's the perfect approach. Advantages/Disadvantages
    • The correct way how to setup a border-box is this (you can find more info here):
    html {
box-sizing: border-box;
}
*, *:before, *:after {
box-sizing: inherit;
}

    But in general, the solution looks really good. And was happy to review it 👍

    Have a wonderful day.

    0
  • Nathan Ro•10
    @nathanro09Submitted over 1 year ago

    Overall, I think I did pretty well.

    Blockers:

    • I keep forgetting the values for SVG viewBox. I wasn't able to successfully use <i> or <svg>, so I just stuck with <img>
    • Had some trouble with the overlay positioning
    • Spent an unreasonable amount of time trying to figure out whether or not the font-weight for "Jules Wyvern" was 300 or 400 (still unsure)

    Please let me know if there are any discrepancies or if you have any advice for me to improve my code.

    HTML FLEX AND CSS SOLUTION

    1
    Radek Tomasek•160
    @radektomasekPosted over 1 year ago

    Hello Nathan 👋,

    had a chance to have a look at your solution and let me first walk through the items you mentioned in the blockers section:

    • You should be able to use SVG in a static document. Checkout this article. Just one note, don't use <i> tag. I know this is sometimes utilized when people are using Font Awesome, but i != image/icon. It's an old tag pre HTML5 which was used for italic texts. It's used with icons but it shouldn't. https://stackoverflow.com/questions/17471390/how-to-use-i-tag-with-icons. Using an SVG or img should work. If an SVG doesn't work, double-check the export.

    • The overlay position. Do you still have a problem? I looked into the preview site and it looks great.

    • Re: "Jules Wyvern": I have access to figma and the particular section should be 400.

    And here is a bit of an observation

    • CSS looks good, but I think you should consider utilizing CSS variables to reduce some duplicities.

    • You shouldn't use absolute values for font sizes. The reason is accessibility. Here is a good article about it.

    • You might also double the line-height. The frontend mentor challenges usually have some tweaks in it. If you specify it, use a ration value rather than a specific unit. Another good article related to this.

    But in general, you had done a great job, especially as this has been your first challenge solution.

    0
  • Radek Tomasek•160
    @radektomasekPosted over 1 year ago

    Hello AomSirawit 👋,

    well done with the challenge, especially as it's your first one.

    Responsive design

    Let me first answer to your first question - how to do a responsive design in general. Well, in this particular case (QR code component), you don't need to worry much about the mobile/desktop sizes. The component size should be unified across any device screen.

    But in general, my recommendation is to start thinking about your layouts with a mobile first approach. Which means, from practical point of view, you start with the smallest possible design (mobile screens) and all the styles you write down, will apply to that. And once you are done, you start adding media queries to your styles and try to address the tablet/desktop sizes.

    The media queries for this approach will have min-width... It might take a while to fully understand that. but it's definitely a worth to do it.

    here is a good article explaining this concept for you: https://zellwk.com/blog/how-to-write-mobile-first-css/

    You had used max-width which is usually used when you work with desktop first design (other direction). It is a good approach too, but I personally find it more difficult, especially for beginners. The reason for that is that the desktop versions usually contain more elements. It's important to position them well, but then you can be more overwhelmed, as you try to style the stuff for smaller displays.

    If you apply the mobile first approach, you will have much better time as a developer.

    General feedback

    I also checked your style sheet and here is a general feedback which can help you make your styling better.

    • Don't use px (absolute size) for fonts sizes in particular - when you use pixels, you prevent users to adjust the font size in case they have some especial disability needs. They can adjust the default font size in the browser and your styles have to react to it. Use rem or em units instead - https://www.freecodecamp.org/news/why-use-rem-to-set-font-size-in-css/.

    • you can also consider using relative units in other places where you normally use pixels.

    • In your HTML template, you are using an img for QR code which is not there for a decorative purpose. The alt attribute should be populated by some text to inform the user about the meaning of the image (in case the image is disabled).

    • you can use CSS properties aka CSS variables for your repeating elements, colors in particular. They are easy to implement and make your code more maintainable: https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties

    Good luck with other challenges 🍀

    0
  • Aghnia•30
    @aghniputraSubmitted over 1 year ago

    This is the third Frontend Mentor challenge that I completed. The most difficult part for me is to center the div that contains social media icons, and I wasn't sure if I used the right code for it. I would be very grateful to receive feedbacks not only about centering the icons but also about anything. Thank you very much :))

    Huddle Landing Page

    2
    Radek Tomasek•160
    @radektomasekPosted over 1 year ago

    Hello Aghnia 👋,

    well done with the solution. I had a chance to have a look into that in the morning and here are my thoughts:

    1: Great thoughts by using CSS Grid for this solution. It helps you to resolve some challenges you would face otherwise.

    However, I have some suggestions re: grid structure (this would also solve your responsibility issues on the small displays).

    • on a small display (mobile layout) I would go for the following structure: 1 column / 3 rows. In each row the logo, illustration image and the block with the main copy are placed.

    • on a bigger displays (tablets and above) I would recommend to use the following structure: 2 columns / 2 rows.

    The key point here is to put the logo in a separate grid elements (on a small display, it will be 1 column/1 row, large one it might be spread in two columns/1 row).

    It will help you to reduce the space between the logo/illustration image and have more flexibility in styling.

    I had personally chosen slightly larger grid than I recommend you here, which gave me even more flexibility (I spread the illustration image in two cells), but even you implement a small change I recommend, you will definitely get some extra flexibility.

    2: It's nice to see you are using relative unit for the font sizes. You can also consider using CSS variables to reduce the duplicates. There is also some commented code inside of your CSS. I recommend to remove it to make things cleaner. You can always check things back, especially as you have things versioned by GIT.

    3: You layout is slightly broken on small displays. Either consider the solution with the update of the Grid structure (the first point) or apply a min-width which help you persist some consistency. It's good idea to implement that anyway in cases the resolution is smaller than the 375px.

    But other than that, well done 👍

    Marked as helpful
    1