Skip to content
OverviewSolutions
0
Comments
13

Declan

@declanslevin300 points

I'm currently a Test Lead for Red Badger. I have > 6.5 years experience as a QA and I'm now looking to move into software engineering. I attended a Front End Web Development course at General Assembly in London and was taught by Matt, the founder of FrontEndMentor, which is how I ended up here :)

I’m currently learning...

JavaScript React

Latest solutions

No solutions submitted yet.

Latest comments

  • Declan•300
    @declanslevinPosted over 5 years ago

    Hi Lukas,

    Good job on your first submission :)

    The first thing I noticed on mobile is that your content gets cropped slightly by the edges of the screen.

    You can fix this in two steps: 1. By wrapping a 'container' div around your body content and adding some padding/margin to this container, which will give you some space either side of your content. You could also just apply the padding to <body> but this can introduce issues if you were to apply a background colour/gradient/image. It's considered good practise to use a separate div as a container instead.

    1. Then you will need to remove the 'fixed' width values on your card divs - you can use max-width instead of width and the card will adjust its width based on the amount of space available rather than push content off-screen.

    The content inside your divs could also use some padding as it currently reaches right to the edge.

    You should definitely have a go at making it responsive next by adding in styling for larger screens using media queries :)

    2
  • Declan•300
    @declanslevinPosted over 5 years ago

    For your use of flexbox, it looks like it's probably not working because you need to set display: flex on the parent container for the elements you want to position. So in your case it would need to be on the wrapper around the icons.

    A fun way to get to grips with the basics of Flexbox is a game called Flexbox Froggy. If you want to become a Flexbox master then you can give Flexbox Zombies a try :)

    1
  • Declan•300
    @declanslevinPosted over 5 years ago

    Hi sroque,

    You could try taking a look at w3schools media queries or CSS-tricks Media Queries.

    A good approach to take is mobile-first: build your page for mobile first, then add media queries to style your page appropriately for larger screen sizes. Once you are happy with your mobile layout, you can add a media query such as @media (min-width: 376px) { } and anything you add in the curly brackets will be applied for screen sizes at or above 376px.

    An easy way to test for mobile responsiveness is to use the Chrome Dev tools (right-click on a page element and select 'Inspect' or Cmd+Option+J (mac)/Ctrl+Shift+J (Windows)). In the top toolbar next to the 'Elements' tab, there is a device icon - click this and it will allow you to emulate different screen sizes. You can select the desired size at the top of the emulator or drag the edge to manually adjust.

    It's a good idea to get your layout right for key screen sizes (375/1024), then drag the screen edge back and forth to see how your elements respond. This will quickly show you where your layout is breaking.

    1
  • Declan•300
    @declanslevinPosted over 5 years ago

    Hi exnihilo0912,

    Great job, this looks really good. Your code is really clean and well structured.

    The only thing I can suggest is maybe you could add a hover effect to the 'learn more' buttons.

    Keep up the good work :)

    2
  • Declan•300
    @declanslevinPosted over 5 years ago

    Hi Prem,

    This looks really good!

    I did notice though that your page responsiveness breaks between 400px and 1050px. I think partly this is because you have fixed widths on your form elements - I would suggest using max-widths instead. Alternatively you could use a percentage width on the parent container and set all the child elements to be 100% width.

    It's also a bit odd that you have nested everything inside the <header> element. You probably don't even need the <header> element because there is no obvious header on the page design.

    I recommend you take some time to fix the issues highlighted in your site report. Otherwise, great job!

    2
  • Declan•300
    @declanslevinPosted over 5 years ago

    I'm curious what you're not happy about, it looks great!

    It's a matter of personal preference, but something I like to do with SASS is nest my media queries in the class declaration - something like:

    .page-container {
  display: flex;
  line-height: 1.5;
  flex-direction: column;
  justify-content: space-between;

  overflow: auto;
  height: 100vh;
  color: white;
  padding: 40px 80px;

  @media screen and (max-width: 675px) {
    text-align: center;
    padding: 2rem;
    height: 100vh;
  }

  .top-row {
    display: flex;

    @media screen and (max-width: 675px) {
      margin-bottom: 15px;
    }

    img {
      max-width: 100%;
      height: auto;

      @media screen and (max-width: 675px) {
        max-width: 50%;
      }
    }
  }

    You end up with more media queries, but this way your original declaration and media query declaration are right next to each other. When your .scss files end up being hundreds of lines long, if you need to change something, you don't need to keep scrolling up and down to change the value in 2 places :)

    1
View more comments