Skip to content
OverviewSolutions
4
Comments
34
Rostyslav Nazarenko
@rostyslav-nazarenko

All comments

  • Udy-Johnson10•10
    @Udy-Johnson10Submitted almost 2 years ago

    I had difficult with aligning the prices to the center and the right side height wasn't aligning with the left. I'm not sure of the positioning code that why I didn't use it. Also not sure whether the site adapts to any display screen properly.

    Product preview card component

    2
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hello! Nicely done!

    • HTML
      • use main and footer for semantics. Change <div class="section"> to <main class="section">, and <div class="attribution"> to <footer class="attribution">.
      • look at the picture element for providing different images to the different viewport widths, plus it has a lot more uses, check it up.
      • don't use id for styling, use only classes.
      • look at providing additional information for people who use screen readers as they will not understand what two prices mean. I'm still learning about accessibility...

    Your CSS has some issues, keep learning and you will notice them yourself. Feel free to look at other people's solutions for learning purposes.

    Keep coding! Have a great day!

    Marked as helpful
    1
  • Konstantin Kamenskiy•50
    @Grendaizo90Submitted almost 2 years ago

    Still have a problem with responsive behavior, maybe you could tell me about bad practices in my code, and about best pratices i should use.

    Thanks in advance!!!

    Order summary component

    2
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi!

    Everything looks great! Only a few suggestions.

    • use resets for button as it doesn't inherit font properties from parent elements.
    • provide a more descriptive alt attribute for the image or leave it empty
    • use relative units in media queries, preferably em units. The same goes for component max-width
    • don't use strong for styling, use CSS.
    1
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Great solution

    There're issues to resolve and a few suggestions

    • HTML
      • alt attribute should describe the image or its function, and be empty if is only decorative. alt="Card Hero Image" gives no information. Don't use word image in alt, screen readers will read img as Image, Card Hero Image. The same with the music icon.
    • CSS
      • I wouldn't recommend splitting CSS into three files, especially in such a small project. The server will have to make additional requests to fetch each file. Look at Sass/SCSS for this.
      • Don't use this trick html {font-size: 62.5%;}. Read this article for more detailed information.
      • Setting max-width: 144rem; margin: 0 auto; to .container makes no sense as it does nothing.
      • Font size is too small, in the style-guide.md body text is 16px
      • Use resets, like displaying images as block elements, and most important making buttons inherit font from parent elements because font-family is right now set to default Arial

    Keep coding!👍

    0
  • Mohamed Taman•80
    @mtamanSubmitted almost 2 years ago

    What did you find difficult while building the project? -- I learn that <picture> git line height from the parent - I had white space after <picture> and I fix it with add line-height: 0; to the parent tag.

    Do you have any questions about best practices? it will be nice to know Your feedback.

    Thank you

    Product preview card component solution

    1
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Great result! 😀

    Just a few suggestions/issues. I'm only studying so take my advice with a grant of salt.

    • component breaks from 700px to 900px, limit the width of the component or set media query to trigger earlier.
    • img is an inline element, most people use resets to make it display as block element that way there's no need in setting line-height to 0, especially when you need then to set it again on text elements
    • for centering elements we use flexbox but it doesn't work if the parent element has no height, so that is why we set min-height: 100vh; to the parent element. You don't need to repeat it on body element and on main element, use only on main. And remove min-width from both of them
    • Sass is a great tool but be aware of too much nesting. It creates problems with specificity in big projects. .prouduct_outer .prouduct_body .old_price {} is the same as .old_price{}.
    1
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Nice and clean code!

    • change <div class="container"> to <main class="container">`
    • use relative units for the width of elements and for media queries (inside the picture element too). Change the font size in the browser to 18px and you will see the component break.
    • change height to min-height on the body element.
    • change overflow-x: hidden; to overflow: hidden;
    Marked as helpful
    0
  • Zeke-Cachi•160
    @Zeke-CachiSubmitted almost 2 years ago

    NFT preview card, made with HTML and CSS only. Had to use a bit of sass to make the eye and cyan color appear at the same time on hover, since I couldn´t figure out how to place the selectors with vanilla CSS to make it happen

    NFT preview card component - HTML and CSS only

    1
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Fantasticsolution

    • wrap you code in main element, and use h1 instead of h2
    • use min-height on body
    • use relative units for media queries, preferably em
    Marked as helpful
    0
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Awesome solution! 😁

    • use relative units instead of absolute, especially for font size
    • don't restrict the height of the html element`. Part of the card is cut. Try turning off and on style in dev tools.
    • I wouldn't separate CSS files
    1
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Great code!

    • Wrap in main element only card and leave attribution on its own in the footer tag.
    • Use relative units for media queries too. If you change the font size in the browser setting you will see that the component will break, 18px is enough to see that.
    • visually-hidden text is the same

    Have a wonderful day!

    Marked as helpful
    0
  • Vas•110
    @VasJMSubmitted almost 2 years ago

    This is my first Frontend Mentor project and it was heaps of fun since it's been a while (read: 4 years) since I've built anything! Who knew trying to figure out spacing and font-sizes from just a jpg was going to be this hard? 😂 I'm sure I'll get better at it!

    I didn't come across any issues, but if someone could comment on how I structured and annotated my code, that would be great! I just want to know if I'm on the right path here. Although if there are any other issues, feel free to point those out, too! :)

    ETA: Okay, so I just saw the FM image preview and it looks all wack, which is weird considering it looks perfect on my browser :(

    ETA: I figured out the issue! Well, not an issue really. It's just that the default font size on my browser is set to 12px (my personal preference) instead of 16px, hence the supersized look. Should I change this?

    ETA: I fixed it!!!

    Responsive Product Card Component using CSS Grid

    1
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi, Vas! Your code is spectacular! 😍

    There is one issue that I found and trust that was hard because everything is perfect.

    Change px to rem or em for the max-width of the card, for media queries, and for a media query in the picture element. For media queries is better to use em. Why do I consider it an issue? If you slightly increase the font size in the settings card breaks.

    Marked as helpful
    0
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi!

    A little bit of the same as in the previous challenge.

    • use main and h1
    • img element should always have an alt attribute, if the image is purely decorative leave it empty alt=""
    • use relative units for font-size
    • font-weight should be without units (line 91 of style.css)
    • for space between elements use margin not padding

    Keep coding!

    Marked as helpful
    0
  • Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Cool code!

    I have a few suggestions.

    • HTML
      • use main element for accessibility. Change <div class="container">...</div> to <main class="container">...</main>
      • alt attribute should describe an image or its function. Don't use the word image because screen readers read the tag as "image". In this case use QR code to frontendmentor.io. Read this article for more information
      • use h1 instead of h2, don't skip levels
    • CSS
      • use relative units to make your solution accessible
      • set max-width: 100% on images

    Hope I've been helpful!

    0
  • ClaudioAmareno•230
    @ClaudioAmarenoSubmitted almost 2 years ago

    Hi! I'll be glad for your review and tips how to improve my code in html and sass :)

    Dancing card

    1
    Rostyslav Nazarenko•560
    @rostyslav-nazarenkoPosted almost 2 years ago

    Hi! Amazing solution! 😍

    I will try to provide feedback, but it is mostly tiny bits here and there.

    • It is not recommended to set `html { font-size: 62.5%; } read more about it here. I'd recommend reading this entire article.
    • Change height: 100vh to min-height: 100vh. If users set a slightly bigger font size they wouldn't be able to scroll and see the whole component.
    • Look at reports. In a real project, you wouldn't use h1 for this but here it is a stand-alone project. Remove p element from button.
    • Use em for media queries too.

    Have a nice day!

    Marked as helpful
    1