Skip to content
OverviewSolutions
27
Comments
10
Dusan
@DEmanderbag

All comments

  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey Gizem,

    Your code looks clean and good love the use of SCSS and BEM.

    I've noticed one thing that you can change for future projects. When you are creating a tags try using <a href="#"></a>. This is a common practice for links that don't go anywhere like in this case.

    Your current solution is reloading the page every time you click the link, this is not a problem because the page is small and you don't even notice it.

    Happy coding.

    1
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hello Athreya, great solution.

    Because this challenge is a component you would usually not use the footer here. Footer is used mostly for copyright data or for some useful links.

    I would also look into removing the onclick event from HTML and instead of that move everything to JS. Your approach is not wrong but I think by using document.queryselector you can target any element on the page right from the JS so you can keep your HTML code separated from JS code. Most of the time it's a good idea to have everything related to JavaScript in a JavaScript file. If your code is larger, it would become much harder to follow.

    Keep on coding ^^

    Marked as helpful
    0
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey Pia, great solution. I really love the use of CSS custom properties.

    Your solution has an image with the space at the bottom that is there because the image tag is an inline element. To remove it all you need to do is to put a display block on the image or in your class on the class group.

    The code looks clean and readable I really don't have anything else to add.

    Keep on coding ^^

    0
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey Becki, to align something horizontally you can use margin: 0 on top and bottom and auto on left and right.

    You have a couple of options here:

    • Try looking into Flexbox where you will be able to align it to center horizontally and vertically
    • Or check CSS Grid

    In any of these two cases, you will need to put a min-height of 100vh on your parent element in your case body after that you will need a display of flex or grid on your body as well.

    10 modern layouts in 1 line of CSS

    0
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey, Soransh great submission.

    This is what I would change. Your tablet/desktop view is not full screen. With your current code as it is try adding display flex on the body element with flex-direction column and with justify-content and align-items center.

    The image is also creating some space at the bottom and in other to resolve that you can use width and height on an image tag of 100% so it can grow as much as the parent element in your case div with a class of image. But after that image can look distorted and in other to resolve that you can use object-fit: cover again on the same image tag.

    Would suggest checking Perfectly Sized Images

    1
  • Glen•45
    @chiptaylorSubmitted over 3 years ago

    I employed flexbox in the desktop layout to ensure that the "Learn More" buttons would not be pushed out of horizontal alignment by the paragraph text. I am curious as to whether there are other ways, better ways, to solve this issue?

    Mobile first workflow using CSS Grid and Flexbox

    2
    Dusan•700
    @DEmanderbagPosted over 3 years ago

    Nice work with the design and the code overall. I really like the use of BEM classes for this project.

    The reason why the website is not aligning vertically is because the page is not full screen. If you check your solution in a web browser and hover over a body (or put background color on a body) element you can see that it's not a full page. To resolve that issue, you can put a min-height of 100vh on a body element.

    In your case, you can set min-height on a class "container" and because you are already using a grid you can just use also place-items: center; to align items horizontally and vertically.

    This next suggestion is more for git and GitHub in general than for this project but I think it will be helpful.

    I would also like to suggest that you put some context on your commit messages or at least what did you change on that commit. Instead of "commit update 8" you can write something like "update cards alignment", or "If applied, this commit will update getting started documentation"

    For git messages, I would suggest this article: "How to Write a Git Commit Message - Chris Beams".

    One last thing is to check again the email on Git and GitHub because looks like they are different and because of that on your GitHub it's only showing one contribution instead of 9 for today

    1
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey Dakota 👋

    For desktop, view try using on the following code on a body

    body { 
min-height: 100vh;
place-items: center;
}

    To center cards to the middle of the screen. If you use the code above you can also remove justify-content: center;

    0
  • Dusan•700
    @DEmanderbagPosted over 3 years ago

    Great work Vjekoni website is almost pixel-perfect, I have a couple of suggestions on your solution.

    I've noticed that for background images you were using over and over I would suggest using the class where you would define only once
    background-repeat: no-repeat; background-size: cover;

    Another thing is that for every background image you also create separate @media it would be much easier if all of those desktop images are under one @media query in my opinion

    I'm not familiar with the tilt.js library but for the card, with the title "Soccer team VR" looks like that tilt part of the script is not working. You might want to look into that.

    On mobile hamburger menu is not closing when you click any list item try using classList.toggle("show") on the link item.

    Keep on coding and improving!!

    0
  • Hadi Khalili•150
    @HDKHALILISubmitted over 3 years ago

    Your feedback is always welcome. I will continue to refactor and improve the app.

    HTML, CSS, REACT

    1
    Dusan•700
    @DEmanderbagPosted over 3 years ago

    Hey Hadi,

    Your solution looks great, I like the way how you are showing that the data is loading. And the website seems to respond quickly, but here is what I would change:

    • I would personally make full job card clickable and I would remove that default underline on the "a" tag.

    • You also have two items with the same id tag, in this case, id="full-time-only" the id element should be a unique element on the page and there should be only one id selector for the same name on the page. In this case, I would suggest changing to a class of "full-time-only"

    Keep on Coding!! ^^

    1