Skip to content
OverviewSolutions
1
Comments
6
Milan Dordevic
@milan93djordjevic

All comments

  • Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    I think is pretty good. Only thing I can point out is that I think you don't need that div.container.

    0
  • Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    Wow! That looks awesome! Quick question - Did you use Sketch file? Or you're simply Front-end Guru that needs only .jpeg?

    2
  • Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    Perfect!

    1
  • Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    I think you should try using less divs... For example you don't need that class container... Try to stick with semantic markup as much as you can and do some research on accessibility while you at it... Also I wouldn't (actually I didn't) use grid on cards itself. I finished also this project yesterday. It's not wrong to use it but seems easier with flex. Maybe you are practicing css grid so you try to use it as much as possible? Also that max-width: 2000px doesn't make sense because desktop version says it's 1400-something px if I'm not wrong... So I'd use that max-width instead... Lastly I don't know why you used pseudo::after for top borders? You wanted to try it out? It's much easier with border-top: ; Hope this help you to become better.

    1
  • Milan Dordevic•165
    @milan93djordjevicSubmitted over 4 years ago

    Hey there,

    Just finished my first challenge. I'd appreciate all the reviews from you guys. Especially the bad ones.

    Repo https://github.com/milan93djordjevic/milan93djordjevic.github.io

    Live https://milan93djordjevic.github.io/

    -Briefly- I struggled with .jpeg design so I'm sure that some of the content (most of it) is not even close to pixel perfect. So while reviewing it please focus on "bad practices" or point out good ones and let me know. Looking forward to hear more from you.

    Thanks, Milan D

    Responsive using grid and flex box

    2
    Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    *I just realized I had style-guides file which I didn't use :'D

    0
  • Bgnichol•60
    @bgnicholSubmitted over 4 years ago

    Thanks for the challenge and the community feedback. I used CSS grid for the desktop version and flexbox for the mobile. I hope it worked out or at least came close. This is my first submission, so any pointers are greatly appreciated.

    HTML CSS (Grid) Flexbox

    4
    Milan Dordevic•165
    @milan93djordjevicPosted over 4 years ago

    Visually looks good. But visual aspect is not the only one you should care for. Look more into accessibility issues and dive into semantic markup for starter.

    Btw I finished this same project few hours back and looking trough your code I realized my grid has significantly more columns which I now realized is absolutely unnecessary.. Good job on that grid, simple and sufficient. I like it. Good job!

    Milan D

    2