Skip to content
OverviewSolutions
9
Comments
251
Roman Filenko
@rfilenko

All comments

  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, good job here, but I see a lot of small issues. To debug some of them use rule - * {outline: 1px solid orange;}. This will help to fix overflow problem. A few points for future projects:

    • don't set defined heigth on containers;
    • preferable don't use id's for styling;
    • img { max-width: 100%;} will make all images responsive;
    • it's better to set max-width on wrapping containers instead of just width.
    • use flexbox or css gris for layout, don't use position;
    • for spacing inside use padding.

    Try to fix some issues first, good luck😉

    Cheers, Roman

    2
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, you can use grid instead😉, cos currentrly gap for flexbox isn't supported in Safari yet. Or use negative margin/padding for placing items inside flex container.

    Cheers, Roman

    0
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, try to fix console error - Mixed Content: The page at 'https://react-clockapp.vercel.app/' was loaded over HTTPS, but requested an insecure resource 'http://worldtimeapi.org/api/ip'. This request has been blocked; the content must be served over HTTPS. Probably this is the issue here

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, great work. Looks nice, but I have a few notes:

    • semantic html can be a little better
    • try to avoid deep nesting in css, like body .page .container .card-body h2, it's hard to overwrite and maintain styles.

    Cheers, Roman

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Monica, nice try, but there some issues you can easily fix. First, add your your pattern images as background-image with css, this will fix positioning and overflow issues. And work on previous feedback.

    Cheers, Roman

    0
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, good job. Have a few tips for you:

    • .intro > ul > li > span > span - don't use some many selectors to target elements in css, usually one class is enough;
    • not sure what the issue is with the stars - would probably just use imgs or spans in a list (img as background).
    • you can try mobile-first approach on next project.

    Also you can look into sass or css variables. 😉 Cheers, Roman

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Miguel, great work on this one, clean, nice code and responsive as well. Found few smaller issues:

    • add some transition for menu links(hover effect, add focus states as well);
    • hiding overlay by default, adding transition to it on a show;
    • slider buttons can be buttons;
    • grid properties can be written with shorthand like grid-template-column: 3/4 or using grid area to make it simpler.

    Hope this was helpful. Have a good one, Roman

    0
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Nocolas, nice work. Some other suggestions how to improve:

    • wriite more semantic html tags (footer should be footer tag, a list of icons - also list of links, etc);
    • add desktop version using mediaqueries;
    • hover on burger menu isn't a proper way to show a menu - make it on click, just few lines in js;
    • in .insurance__text section - with z-index: -5 you can't really click a link;

    Keep practicing, Roman

    0
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Romain, good work, but your solution need some improvements. First, I suggest to not position .between block absolutely - you can define negative margin to push it down and add some z-index to make on top of the footer. You can check my solution - I've made similar, a dark challenge and apply some of my techniques from there (grid, hover effects, html structure). Good luck with coding😉

    Cheers, Roman

    0
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hi Grace, congrats, clean and nice code, almost perfect to me 😉

    Cheers, Roman

    2
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hi Artem, you did a great job, mobile looks ok, just a few edits will help with issues you encoutered. You're right about hard-coded values and funky ones like padding-top: -49.589%" - try to avoid them, so my suggestions:

    • first, set box-sizing: border-box for all elements with * selector;
    • don't use fixed values, on containers, for example set max-width, like 1200px;
    • for images it's ok to set width and height, additionally object-fit will help to preserve proper picture proportions.

    Usually, it's better to start with mobile-first approach and then add needed changes with wider viewport width.

    Hope this was helpful😉. Good luck with edits.

    Cheers, Roman

    2
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey, what issue are you implying to ? Here some fixed and improvements you can do:

    • don't use id's for styling;
    • it's better not to set margin, padding in %;
    • set max-width instead of just width on some containers.

    Cheers, Roman

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Dave, good work on this one. You can improve further by optimizing you js code - define variable only once on the top, use one function with if statement inside to not dublicate your code. Also you can wrap the whole code inside self executing function😉

    And work on feedback about styles.

    Cheers, Roman

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hi Lucas, to make all flex-items same size define flex:1 shorthand property. Check this article for details - https://css-tricks.com/snippets/css/a-guide-to-flexbox/. But would rather change techniques - make .card-profile-container as css grid container and layout all elements inside the .card-profile box with flexbox😉

    Have a good day, Roman

    1
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hey Dave, nice work, just a few minor touches left:). To fix your issues:

    • for image to be fullpage set {height: 100%; object-fit: cover;), that should do the trick. Also check if all grid properties are applied;
    • form: make it as flex container, and just position button to the right {right: 0, disable funky left value};
    • add some transition for btn on hover effect.

    Overall, would advice to use Firefox, devtools are very usefull, specially when debugging flex or gird problems, give it a try😉 Cheers, Roman

    2
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hi Błażej, good job, but with a few corrections can be a lot better. I was wondering about choices you made, so here a few tips for improvement:

    • please don't use !important, id's you already mentioned;
    • add max-width to .container and center it;
    • fix img distorted proportions(max-width: 100% for all imgs, object-fit:contain as option for big one image);
    • why social icons have absolute position?
    • what the purpose of address in footer? Hope this was helpful feedback for you, have a good day.

    Keep practicing, Roman

    2
  • Roman Filenko•3,335
    @rfilenkoPosted over 4 years ago

    Hi Artem, great work on this one. As for screenshot - it probably because puppeteer (would guess in this case) creates screenshot before yor styles kickoff. Have a few notes though:

    • tags could be more semantic;
    • progressbar is rather hard to style, add vendor prefixes to make it crossbrowser;
    • some strange values like height: 201px; width: 918px; margin-bottom: 34.2px; - a bit lack of consistency here, but you probably used all values from sketch file;

    Cheers, Roman

    2