Skip to content
OverviewSolutions
2
Comments
18
David Omar
@davidomarf

All comments

  • David Omar•580
    @davidomarf

    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!

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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. :)

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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! :)

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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!

  • David Omar•580
    @davidomarf

    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.

  • David Omar•580
    @davidomarf

    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>
  • David Omar•580
    @davidomarf

    Hey Militus.

    I like the layout. You replicated it pretty well. And I like how it adapts every element to all widths without being funny-looking.

    I only noticed that the tags section is bugged. It displays the language list as a single string: Frontend, Senior, HTMLCSSJavaScript, HTML, CSS, JavaScript.

    I found the line that causes this. It's this one: https://github.com/MilitusInnocent/static-job-listing/blob/master/src/jobBoard.js#L20

    You're adding the languages twice. One as an array, and one more as a spread array.

    The only other thing I'd do would be working on the shadows. It looks very dark and strong.

    Try with the darker blue. The same that you used in the tags text. And give it a little bit more of blur.

  • David Omar•580
    @davidomarf

    Hey!

    That's a nice touch to add.

    I see you're using Vanilla JS. I'd create an eventListener in your background element.

    But that event will be triggered when you click on any of its children. This is event propagation. I don't know of useful resources to read about it, but this one looks fine.

    For example, if you have this layout:

    <body>
  <div class="first-level-child">
    <div class="second-level-child">
    </div>
  </div>
</body>

<scrpt>
  const body = document.getElementByTagName('body')[0];
  body.addEventListener('click', function (event) {
    // event holds information about the event
    console.log('clicked');
  });
</scrpt>

    When you click inside body, it'll log 'clicked', as expected.

    But if you click on .first-level-child, or .second-level-child, it'll also log 'clicked'.

    To avoid it, log the event.target, and event.currentTarget, then see how could you use them to determine when the click happend on the body, or in any of its children. :D

    (event.target and event.currentTarget are DOM elements. So you can also compare them with elements you got via getElementBy....)

    Good luck, keep the good work.

    EDIT: I misspelled script to avoid it from being hidden on the comment.

  • David Omar•580
    @davidomarf

    Hey there! Nice solution. You nailed the layout and responsiveness.

    I like this challenge because it makes it easy to give one tip of something I just learned recently:

    In your css you have three border-radius properties for the inner boxes:

    .top-row{
  border-radius: 7px 7px 0 0;
}

.box-left{
  border-radius: 0 0 0 7px;
}

.box-right{
  border-radius: 0 0 7px 0;
}

    But it can be replaced easily by adding it to the main container, and hiding the overflow (otherwise, you'd end up not seeing the border-radius):

    .main-box{
  border-radius: 7px;
  overflow: hidden;
}

    Having things like that in a single place makes your code easier to maintain, change, or grow.

  • David Omar•580
    @davidomarf

    Pretty clean architecture. :) I like that you keep your components small and truly "atomic".

    I only noticed one issue, and it's that if you click anywhere, as long is it's at the same height of the share button, the hidden menu gets activated.

    One fix for that is placing it inside the same container as the Author element.

    Another one is:

    • Make the white container position: relative;.
    • Remove margin: auto and left: 0 (this one gets added with .inset-x-0) from the button container (the one with the classes "absolute z-30 inset-x-0 md:mb-2 w-full h-16 m-auto pr-10 bottom-0 cursor-pointer flex justify-end items-center", I cannot find a name to specifically refer to it, haha).

    I haven't tried tailwinds yet. Was it good in terms of "development experience"? Still looks cryptic to me, haha.

  • David Omar•580
    @davidomarf

    Hi José!

    I'll get into some React tips instead of your layout.

    I saw you heavily use setState in your card.js component. And there's really no need to. All of that could have been replaced with props and some logical operators.

    And since you wouldn't be using state in your component, you could write it instead as a functional component!

    Using functional components will result in cleaner code, since you don't need a lot of boilerplate code that comes with using Class components.

    For example, card.js could be written like this:

    const icons = {
  tw: IconTw,
  ig: IconIn,
  yu: IconYu
  //... all other icons
}

// This "{ title, ... }" notation is "spreading". It allows you to declare the
// props without you needing to manually say "props.title".
function Card( {  title, type, insight, subtitle, upgrowth } ) {
  return (
  <div className="container">
  // Some other elements... 
    <div className="icon">
      <img src={icons[type]} />  @nathanf
    </div>
  // Some other elements...
    <div className="col">
      <div className="card-insight">
        {insight}
      </div>
      <div className="subtitle">
        {subtitle}
      </div>
    </div>
  // Some other elements...
  </div>
  )
}

    Also, generally you don't want very complex logic in your JSX, but some is fine.

    For example, you have

    <img src={this.getTypeIconGrowth()} className={(this.props.upgrowth <= 0) ? "down":"up"}/>

    You're using a function and a logical expression for almost the same thing.

    I'd rewrite it like this:

    <img src={ upwroth <= 0 ? iconDown : iconUp } className={ upgrowth <= 0 ? "down" : "up" }/>

    Also, you could have used just the plain type or title props to set typeCardNetwork and classNetwork.

    For example, instead of setting those two, you could just have done a workaround with something like:

    <div className={ type ? 'card' : '' }>
  <div className={ type }></div>
</div>

    The { type ? 'card' : '' } will check if type is "truthy" or "falsy". An empty string, null, or undefined are all "falsy", so it'd end up being ''.

    And in yout .scss, you could style whatever you pass as type (which I'd also suggest, should be more expressive and meaningful: yt -> youtube, im -> instagram, etc...)

  • David Omar•580
    @davidomarf

    Hi, Suren!

    I'm not sure what you mean with code structure. If you mean the placement of files, directories, names, etc. I don't see any trouble. You don't have many files to really require something "special".

    If you mean the actual code, I have one tip regarding border-radius.

    • In your style, you have these properties:
    .firstPart {
  border-radius: 8px 8px 0px 0px;
}
.secondPart {
  border-radius: 0px 0px 0px 8px;
}
.thirdPart {
  border-radius: 0px 0px 8px;
}

    And it works as you expect, but if you need to ever add more parts, or change the layout, you'll have troubles updating all those border-radius.

    Instead, I'd use a single property on the parent of all those Parts:

    .form {
  border-radius: 8px;
   
  // This is what makes it work. If you don't add it, you'll 
  // end up with no visible border-radius.
  overflow: hidden; 
}

    Nice work, keep it up!