Skip to content
OverviewSolutions
2
Comments
18
David Omar
@davidomarf

All comments

  • David Omar•580
    @davidomarfPosted about 4 years ago

    Hi!

    I noticed some things.

    If I add some things to the filters, and then manually remove them by deleting one by one, the list doesn't update. Even if I end up with 0 filters.

    When the list is pretty small, the background color doesn't cover the whole body, only some part of it.

    The "Clear" button isn't vertically centered.

    In widths between 768 and ~1080, the layout for the individual offerings start to overlap within itself. Maybe you could set the media query to a greater width, or make the "wide" layout more responsive.

    One tip is to align the tags (Frontend, Senior, HTML CSS...) to the right instead of the left.

    Nice animations on the filter bar! I also loved the cursor for the hoverings!

    The layout is good in general.

    I'll go to sleep, but I'll try to check the code tomorrow morning and offer some comments about it.

    Keep the good work!

    2
  • David Omar•580
    @davidomarfPosted about 4 years ago

    This is awesome! I'll be visiting your solution to get some ideas about the positioning of layouts like this. I've never done anything similar.

    1
  • David Omar•580
    @davidomarfPosted about 4 years ago

    Hi!

    At first I noticed that the content was being contained always at the center even in wider screens. That's good!

    I realized that there were some text elements that had <br> in there just to make some vertical space.

    A more convenient way to do it, is by limiting the width of the text container. Then if the text grows bigger than that width, it'll wrap inside it, instead of growing sideways like crazy.

    I also noticed some horizontal scroll in all widths. After searching a little bit, I found out what was causing it. The companies container, and the features container were both wider than the div.content in some small widths. But the footer was always wider, no matter in what width.

    A fix for the footer issue is using box-sizing: border-box on the footer tag. Usually, when you set the width of an element, and then add border or padding to it, it'll be extra space to that width. And sometimes you just expect it to be included in that width. That's what border-box does.

    You can check this guide: https://css-tricks.com/box-sizing/

    And a fix for the features and companies containers, is trying to limit the size of them, and of the 'main' containers, like body and div.content.

    I see you use flex-box, so you can try using flex-wrap. This allows the items inside the flex-box to get into another row/column if there's not enough space to fit them all.

    Try adding this to your ul.companies and ul.features:

    flex-wrap: wrap;
justify-content: center;

    And of course, adding media queries as scorpion suggested.

    Nice work :) It looks polished. The work with the buttons and sections it's pretty good.

    0
  • David Omar•580
    @davidomarfPosted about 4 years ago

    Hi Luis!

    Good layout. It responds well to resizing.

    There's one detail in your div.discount. In the full-width style it only has vertical padding. And there's a resolution in which the text in the div has the same width than the div, and looks like it has a bad layout.

    You could improve the input validation. If I try to put 'a' as email, it doesn't complain.

    Overall, good work. :)

    1
  • David Omar•580
    @davidomarfPosted about 4 years ago

    Hey!

    You should try to wrap your solution in another container so it is contained in a limited space, and doesn't cover the whole width of the screen.

    And remember to add border-radius for the round corners!

    Nice use case for grid! I've always found it difficult.

    0
  • David Omar•580
    @davidomarfPosted about 4 years ago

    This is an amazing solution! One of my favorite ones so far. Nothing I could add.

    Very responsive, perfect layouts for different media queries, and in general, done with best practices.

    Congrats! :)

    3
  • David Omar•580
    @davidomarfPosted about 4 years ago

    The mobile version scales perfectly, and the desktop version works great on the intended width, but looks funny on the intermediate widths.

    Also, the social icons appear at the middle of the screen when the height of the viewport is higher.

    I think you could achieve a more responsive layout if you separate the "main" containers in three: The navbar, the content, and the footer. Then you display them in a flex container with a height of 100vh, and that justifies its content as space-between in a column.

    In tall screens, the background looks cropped. To prevent that, change background-size from contain to cover.

    I really like that your content is capped to a maximum width.

    2
  • David Omar•580
    @davidomarfPosted about 4 years ago

    The final result works flawlessly.

    I like the animations of the social buttons.

    I was wondering why did you use border-radius as a mixin, instead of using the plain property, but then looked at the cross-browser file. I didn't know border-radius had to be treated like that before seeing your code, haha.

    I have only one tip:

    Instead of setting the border-radius for the child components, just set it in the parent, and hide the children overflow with overflow: hidden.

    The 'complexity' reduction is not very much noticed in here because you don't have that many children, but knowing it may save you multiple border-radius: x x x x; in the future.

    2
  • David Omar•580
    @davidomarfPosted about 4 years ago

    Wow, definetely gonna steal some tricks from your SCSS file.

    I haven't used mixins yet. And just by reading your code I think I "got it".

    The only thing that I'd change, is the order of the properties. I like this approach:

    https://webdesign.tutsplus.com/articles/outside-in-ordering-css-properties-by-importance--cms-21685

    >

    The order I use is as follows: 

Layout Properties (position, float, clear, display)
Box Model Properties (width, height, margin, padding)
Visual Properties (color, background, border, box-shadow)
Typography Properties (font-size, font-family, text-align, text-transform)
Misc Properties (cursor, overflow, z-index)

I know that border is a box-model property and z-index is related to position,
but this order works for me. Even though I don't like alphabetical ordering,
there's something that just feels right about putting z-index at the end...

    But it's just an opinionated comment.

    Great solution and great use of transitions.

    2
  • Robert McGovern•1,075
    @tarasisSubmitted about 4 years ago

    First attempt at anything like this.

    Spent far too long using diffchecker to compare the design image and an output grab from chrome. You can see mob-img-diff-1.png & img-diff-2-desktop-near-enough.png in the work-through directory in the repo to see how close I ended up (there is some mild diff because of the image quality, the illustration mockup has slight diff colors in it, likewise the background)

    With positioning like this, is it better to use %, rem, or px?

    I tried using 700 weight for the header "Build the ...." but it never looked right. Nor does 400 but it's close enough. What should have it been?

    How best to get the fontbook-awesome images more central in the circles?

    How close should I be looking to get?

    Huddle Single, CSS and HTML (Desktop first, Mob second)

    3
    David Omar•580
    @davidomarfPosted about 4 years ago

    Hi Robert.

    This is really the closest I've ever seen someone replicating a layout. Kudos for that dedication and attention to the details.

    I didn't know about the existance of diffchecker. I'll give a look to it.

    And definetely documenting your process is really useful. It's something not everyone does, but should.

    About the website, I think it's better to take what a design "means", than what it literally looks like.

    For example, the design probably means that, in wide formats, the page should cover all the screen. Both in height and width. And that is a more powerful meaning that "The image should be Xpx wide and Ypx tall. This text should be Xpx wide and Ypx tall. There should be a space of Xpx between these two eleemtns".

    Along those lines:

    Try to limit the width of your components and to contain them at the center of your screen.

    If you're in Chrome, open the console with Ctrl + Shift + I and then press Ctrl + Shift + M to open the "Device Toolbar". With that, you can set custom resolutions and see how your page would look in a screen of that size.

    In your page, if you go to really wide resolutions, the text will appear in only one line. And it starts to lose its expected design.

    To prevent that, you can create a main container that will hold everything inside it. Then limits its size, and automatically center it.

    My favorite way to do that is with something like this:

    <body>
  <div class="main-container">
  </div>
<body>


body {
 display: flex;
}

div.main-container {
   margin: auto; // This centers it horizontally

  // This limits the width. 
  // So in ultriwide screens, it'll not grow any bigger than this.
  // And therefore, will keep the layout (e.g., the text not overflowing)
  max-width: 1280px; 
  
  // This is what the width will be on screens smaller than the max-width.
  width: 90%;
}

    I see you use flexbox, but some of the child elements of flex-elements are not restricted in the size they can take. This is useful to maintain proportions in different sizes.

    For example, to avoid the image taking more space than it should, you could limit its width. Just as before. And you can set it to percentages, too. And those percentages will be relative to what their parent width is.

    <div class="some-container">
  <div class="image-container">
    <img src="">
  </div>
  <div class="other-container">
  </div>
</div>

div.image-container,
div.other-container {
  // This will make both containers be 50% of the size of some-container.
  // It doesn't matter what's the width of some-container.
  width: 50%
}

div.image-container img {
  // This doesn't mean it'll take all the width, 
  // but all the width of image-container (which is half the size of "some-container") 
  width: 100%;
}

    And finally, check your "intermadiate" widths.

    Try not to restrict your designs to two resolutions. Make it work for in-between widths.

    Set at 650px wide, for example, only the image is visible. And it's overflown to the right.

    You can use flex-box, max-width, and width for that.

    Congrats for your solution. I'll definetely give diffchecker a try. It looks like this solution had a lot of effort behind it. Keep the good work!

    3
  • David Omar•580
    @davidomarfPosted about 4 years ago

    Hey! The desktop layout it's pretty good. But in resolutions below 1284px, the image starts to overflow and becomes less and less visible. Then, around 1040, the image gets duplicated and fills all the width of the screen, looking streched.

    You're displaying both the hero-desktop and hero-mobile images.

    The only other detail, it's the button. It looks a liiiiitle bit bigger than the text field. In the prototype they're the same size.

    Try to play with the property box-sizing: border-box, and try to manually set the height of both the input field, and the button.

    1
  • pogov•50
    @pogovSubmitted about 4 years ago

    Hello. In this project I chose to navigate to the details page using Redirect from react router. I had some problem when I wanted to redirect from one country details to another country details page. I had to use different path for that kind of redirects. Does anyone knows If there is a better way. I feel like this is not the most "elegant" solution. Thanks.

    Countries Info App

    1
    David Omar•580
    @davidomarfPosted about 4 years ago

    Hey!

    Yes, there's a more elegant and idiomatic solution: Link.

    It's a component that when clicked, will automatically redirect you to what you tell it to. Without weird handlers that render a <Redirect>.

    Then you move your styles from the div.whateverClass to a.whateverClass.

    <Link to={`/countries/${country.name}`}>
    <div className={styles.flag}>
        <img src={country.flag} alt={country.name} />
      </div>
      <div className={styles.info}>
        //...
      </div>
</Link>
    1