Skip to content
OverviewSolutions
8
Comments
10
P
James
@james-work-account

All comments

  • James•340
    @james-work-accountPosted about 2 years ago

    A couple of immediate things:

    • In Firefox, your radio buttons are still visible (albeit tiny). You can set something like -moz-appearance:none; but this will make keyboard navigation look a bit odd. Worth investigating.
    • There isn't any feedback that the selected radio button worked - it would be good to keep the active radio button visibly selected.
    • Your "Rate us again" element needs to be a <button> not a <div>. You should never have interactive elements which aren't buttons, inputs or links.
    Marked as helpful
    0
  • James•340
    @james-work-accountPosted about 2 years ago

    This looks great! You have a few accessibility/usability issues though:

    • You need to use buttons or links for interactive elements. <img> tags shouldn't ever have onclick listeners. The most basic way to test if your site is usable by people with accessibility needs is to see if you can tab through the interactive elements or not (ie try to use your website without your mouse/trackpad) - on your solution, you currently can't
    • Missing a <main> tag
    • Your images don't have accessible names (aria-label or sr-only text)
    • Your viewport <meta> tag is wrong, so accessing on a phone looks really small/awkward. You should change <meta="viewport" content="width=device-width, initial-scale=1.0" /> to <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    • When adding in a proper meta tag your styling will need changing as it looks a bit messed up for me when I try to use it from a mobile device with the correct viewport tag
    • You're missing the "Rules" button/popup
    Marked as helpful
    0
  • James•340
    @james-work-accountPosted over 2 years ago

    Looks great! Just a handful of of things I noticed:

    • Your website overflows between ~769px and ~843px, you should adjust your media queries to ensure there isn't any unintentional overflow.
    • All elements with hover effects should be either anchor tags or buttons, even if the anchor tags have a href of # or the buttons don't have an associated function. So in your case, your Facebook, Twitter and Instagram icons should be wrapped in anchor tags (you can probably just replace <div class="social-icon">...</div> with <a href="#" class="social-icon">...</a>).
    • Your hover effects should also apply to the focus-visible pseudo-class. This means that people who mostly navigate websites using their keyboard rather than mouse (such as myself) can have a visual indication that they've tabbed to the correct element. So for example, .register-btn:hover should be .register-btn:hover, .register-btn:focus-visible.
    • This is a problem that a lot of people who are relatively new to SCSS face - avoid heavily-nested SCSS. A good rule of thumb is that you shouldn't really need to nest SCSS selectors other than pseudo-classes or generic selectors like .my-class + *. There's really no need to write something like .hero-container .hero-content .register-btn, since your .register-btn isn't being used anywhere else. If you take a look at your outputted CSS, it should end up looking like it was written without SCSS, not like heavily nested spaghetti.

    Otherwise, this looks really clean. Good job!

    Marked as helpful
    1
  • James•340
    @james-work-accountPosted over 2 years ago

    A great start! Some things which would be good to change:

    • Remove the margin on your <body> element to avoid unnecessary vertical scroll. Related: it would be worth looking up a good CSS reset as general "good thing to do when starting a new project", e.g. https://piccalil.li/blog/a-modern-css-reset/ .
    • Change height: 100vh to min-height: 100vh. You don't want to set explicit heights on things since height of an element should be decided by its contents. However if you want something to be "at least X height", use min-height. In this challenge it doesn't matter too much since the component is small, but it's a good habit to get into early.
    • It's great that you've used a <main> element! Something to consider would be to change <div class="attribution"> to <footer class="attribution">`, since a footer is a landmark and your footer is outside your main element. It's all about using the correct semantic element, and this will fix your remaining Accessibility Issue.
    • Consider moving away from px for widths/heights/font-sizes/etc and switching to using rem for this. There are a few articles explaining why you'd want to do this (e.g. this one) - there are online px-to-rem converters if you want to make your life easier.
    • Style on classes not ids (e.g. add a .card class to your #card element and style with the class not the id). A general rule of thumb is that classes should be reserved for styling and ids should be used for things like linking up inputs with labels, making something selectable with javascript, making hyperlinks on a page, etc.

    Some stylistic things to consider:

    • Instead of having a percentage width on your image, leave it at 100% and rely on padding (using rem) to shrink it. Not a major thing, just something to think about (leaving widths alone and using padding/margins/flex/whatever to influence size).
    • I guess you could link to your GitHub or Frontend Mentor profile in your attribution (it currently just links to #).

    Overall though, this is a really solid submission. Keep it up!

    0
  • arey•350
    @arey-devSubmitted over 2 years ago

    I'm glad I was able to finish this design, but I wasn't satisfied of my solution. Also, one thing I've been confused at is the scrollbar. What I did on this project was add padding on the body element to have scrollbar effect, but I'm not sure if that was the best practice. Can you give me some tips or advice? Thanks!

    4 Feature Card Section built using CSS Grid & Flexbox

    2
    James•340
    @james-work-accountPosted over 2 years ago

    Looks great!

    Something to consider, why did you put min-widths on your .feature and .container elements? Any people with phones below ~400px wide (e.g. iPhone 13 mini) will experience horizontal overflow, which is something you'd generally want to avoid.

    Also, consider moving away from px for widths/heights/font-sizes/etc and switching to using rem for this. There are a few articles explaining why you'd want to do this (e.g. this one) - there are online px-to-rem converters if you want to make your life easier.

    Marked as helpful
    1
  • James•340
    @james-work-accountPosted over 2 years ago

    Your solution needs some content landmarks - the .box div should be inside a main element and your .attribution div should be a footer. This is for accessibility reasons, as it tells assistive technologies (e.g. screen readers) where everything is and makes your site easier to navigate for someone with disabilities (plus it will remove a lot of your Report issues).

    This project doesn't really require relative positioning - have a look into positioning elements with flexbox. For example, on my screen everything is off-centre since everything's positioned with px values (have a look at your solution from sizes in-between and either side of the given screen sizes in the design files). You should be able to use flexbox and not need any media queries at all, since the element won't stray from the centre of the screen.

    You may want a max-width on your .box but shouldn't be setting hard widths on elements - for this challenge it doesn't matter as much, but if someone has a very small screen, or they are viewing the website split-screen (with something else next to it), etc, your elements will overflow off the edge of the screen. Same goes with height - you may need a max-height, but generally you'd have the height dictated by the container's contents.

    This is a good start. I suggest you challenge yourself to rewrite your CSS removing all media queries and fixed heights/widths, whilst keeping the solution looking the same. Good luck!

    0
  • James•340
    @james-work-accountPosted over 2 years ago

    Your live URL and code URL aren't working - please make sure you've put them in right

    0
  • James•340
    @james-work-accountPosted over 2 years ago

    This is a great start! Just some notes to improve:

    • Missing box-shadow on the container
    • You have skipped heading levels - there shouldn't be a h3 without a h2, there shouldn't be a h2 without a h1, etc. The h3 should be a h1 and re-styled if it is too big
    • <img> could do with an alt property - it is not obviously described by any other text on the page and isn't purely there for aesthetics
    • It would be good if the card was vertically centred
    • Your container could be a main, your attribution could be a footer. This would remove some of the accessibility issues that the automated tool has raised
    • Some of the padding/font-size values could be tweaked to get it closer to the original design (though honestly this isn't a priority)

    Otherwise this is a really good submission. Keep it up!

    0
  • James•340
    @james-work-accountPosted over 3 years ago

    Looks great! Just a pointer to make sure the items are all the same height on Desktop - try taking align-items: center off the .container class.

    Marked as helpful
    1
  • Fraser Watt•1,790
    @fraserwatSubmitted over 3 years ago

    This was a fun one!! Trickiest part was working with images sizes, definitely the thing in front end I find hardest.

    Still not 100% pleased with the "Graphic Design" / "Photography" section and how that changes at different page widths, so any pointers on how to approach this so the image responds more smoothly (and I don't have to rely on background colours at some points) v much appreciated! 😎

    ☀️ Sunnyside Landing Page 🍹

    3
    James•340
    @james-work-accountPosted over 3 years ago

    A couple of small things I noticed:

    • On mobile view, the Graphic design and Photography <p> tag text is a light grey rather than dark green/blue respectively
    • On mobile, if you open the hamburger and switch to desktop view, the dropdown menu stays open instead of going back to the top

    Otherwise it looks great! I've been struggling to change the colour of the sunnyside logo in the footer, I just resorted to changing the colour in the svg file itself. Interesting solution to use a filter on it, I hadn't thought of that.

    1