Skip to content
OverviewSolutions
3
Comments
11
Joshua
@minimalsm

All comments

  • Joshua•25
    @minimalsmSubmitted almost 3 years ago

    Second frontend mentor challenge completed 🎉

    As this was fairly simple my main goal was to use BEM (never used it before) and have it be pixel perfect.

    When trying to achieve pixel-perfect I took screenshots of the component in the browser and overlaid them in Figma using "Place Image". Is there an easier way of doing this?

    Also, using BEM for the first time. I was unsure if I thought have used a BEM class to style the article or not as the design was fairly simple? Any thoughts on this?

    Had to use Surge for the site because GitHub pages isn't rendering the page properly 🤷‍♂️

    Any feedback is greatly appreciated! Thanks 😬

    Here are some resources I found helpful:

    Make Your CSS Variable Names Suck Less

    Accessible heading structure

    Pixel-perfect QR code component (HTML, CSS)

    3
    Joshua•25
    @minimalsmPosted almost 3 years ago

    @grace-snow all changes implemented, thanks again :-)

    0
  • Joshua•25
    @minimalsmSubmitted almost 3 years ago

    Second frontend mentor challenge completed 🎉

    As this was fairly simple my main goal was to use BEM (never used it before) and have it be pixel perfect.

    When trying to achieve pixel-perfect I took screenshots of the component in the browser and overlaid them in Figma using "Place Image". Is there an easier way of doing this?

    Also, using BEM for the first time. I was unsure if I thought have used a BEM class to style the article or not as the design was fairly simple? Any thoughts on this?

    Had to use Surge for the site because GitHub pages isn't rendering the page properly 🤷‍♂️

    Any feedback is greatly appreciated! Thanks 😬

    Here are some resources I found helpful:

    Make Your CSS Variable Names Suck Less

    Accessible heading structure

    Pixel-perfect QR code component (HTML, CSS)

    3
    Joshua•25
    @minimalsmPosted almost 3 years ago

    aria label isn’t valid on a h1

    Realised that after I submitted. I was fooled by this highly upvoted StackOverflow post. I guess it seemed to the most a11y friendly option, but I'll refactor this to use a sr-only class.

    your BEM is close, but you’ve forgotten to use the block class of card on the article

    Yes! Wasn't sure on this (it was one of my questions). Any thoughts as to using / not using BEM when we're dealing with small components like this?

    line height should be unitless, never in px

    Noted, will refactor 😀!

    letter spacing needs to be in em really. Again, px no good coz it can’t scale with the text of sizes/zoom are changed

    Ah good point, thanks!

    never have a max height on elements containing other content. If I had a larger font size or a smaller device, that max height could break the solution for me

    Lessons learned! Seems stylistically this wasn't doing anything anyway 😅 (at least at 'normal' font-size/zoom)

    Thanks so much for your thorough feedback @grace-snow ❤️

    0
  • Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @ VictorDjun, this is a good start 💪

    Some attention needs to be paid to the details here to make this perfect ☺️.

    Suggestions

    • H2 should be dark-blue (#1F314F), not black.
    • The card needs more width (design is 320px)
    • Instead of setting a width on the image (makes it hard to maintain) you could set the image width to 100% but add padding to the container to achieve the border.
    • Tone down the box-shadow a little (or, a lot 😬)
    0
  • Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @ VictorDjun, this is a good start 💪

    Some attention needs to be paid to the details here to make this perfect ☺️.

    Suggestions

    • H2 should be dark-blue (#1F314F), not black.
    • The card needs more width (design is 320px)
    • Instead of setting a width on the image (makes it hard to maintain) you could set the image width to 100% but add padding to the container to achieve the border.
    • Tone down the box-shadow a little (or, a lot 😬)
    Marked as helpful
    0
  • Sj•120
    @SJ-74Submitted almost 3 years ago
    What are you most proud of, and what would you do differently next time?

    I used SCSS for the first time in any FEM challenge and would like to improve my SCSS knowledge as of now.

    What challenges did you encounter, and how did you overcome them?

    Nothing much as this was very beginner friendly challenge and I have already completed few challenges before this.

    What specific areas of your project would you like help with?

    Would appreciate feedback on use of SCSS.

    Mobile first using CSS flexbox

    3
    Joshua•25
    @minimalsmPosted almost 3 years ago

    Looking good @SJ-74 💪!

    Few suggestions to get it closer to the original design:

    • H1 font-size should be bigger (1.375rem)
    • I find setting padding on the bottom of the card instead of the paragraph text gives better (more flexible) control over the spacing.
    1
  • Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @poorneshvishnole, this is a good start 💪

    Some attention needs to be paid to the details here to make this perfect ☺️.

    Suggestions

    • H2 should be dark-blue (#1F314F), not black.
    • H2 should be centered horizontally
    • H2 and P tags should have an explicit font-size set. Currently, they don't match the design.
    • Card container needs more border-radius (Around 16px/1rem)
    Marked as helpful
    1
  • Fraser Watt•1,790
    @fraserwatSubmitted almost 3 years ago

    more of a timed project this one. 13 mins from start to deployment.

    QR Code

    1
    Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @fraserwat this is looking good!

    Some fixed height on your container and line-height on the text and it'll be perfect 😀💪

    0
  • Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @Mortazamhammed, nice job on this 💪

    Few things I'd change:

    Important ones

    • The '$29 per month' should not be a <h4>, it isn't a heading.
    • You've added padding-left: 10px to your h4 span which is causing the '$29' to also have padding-left.

    Nitpicking

    • The button should have more height to match the original design
    • There is no design guidance for this but "See a hover state on desktop for the Sign Up call-to-action" is one of the stories on this solution
    Marked as helpful
    1
  • Joshua•25
    @minimalsmPosted almost 3 years ago

    Hey @skyv26, this looks great 😀

    A few things I'd change

      1. Your whites are getting washed out by the background-color (#F6F6FD). I think the design calls for using pure white (#FFF).
      1. You have some inconsistent spacing in your markup (e.g. double space here: <article class="article article-2">) which could use cleaning up.
      1. The toggle button's circle should be larger to match the design.
    Marked as helpful
    2
  • Joshua•25
    @minimalsmSubmitted almost 3 years ago

    Completed my first frontend mentor challenge, yay 🎉

    Questions

    • How could I improve the naming of my selectors? I've spent too long in StyledComponents land... I think they're okay but definitely could be better.
    • I feel like the stylesheet could be better organised, any tips here?
    • I originally used a H2 for the 'Join our community' heading. Realised the HTML checker would shout at me if I didn't have it as an H1 but it feels kind of strange having it as an H1 considering this would likely be a component within a page, rather than a page itself?

    Mobile first single price grid component (HTML, CSS)

    2
    Joshua•25
    @minimalsmPosted almost 3 years ago

    Thanks @mcdud,

    • I looked into BEM before but never gave it much of a shot—it seemed too verbose for my tastes. I'll have a look into it for next time :-)
    • Thanks for the tip! I'll refactor this to use that solution.
    0
  • Joshua•25
    @minimalsmPosted about 3 years ago

    Hey :-) just a couple of things I would fix:

    • your headings should be uppercase (you could do this using text-transform: uppercase; so you don't have to hardcode it)
    • on the live version the buttons are in cursive (seems to be the fallback, are the fonts being loaded and referenced correctly?)
    • having your main set to 85vw causes some overflow issues right before you hit the 48rem media query
    Marked as helpful
    0