Skip to content
OverviewSolutions
7
Comments
34
eagermonument81
@ixtk

All comments

  • stephmunez•435
    @stephmunezSubmitted about 2 years ago

    Hey everyone!

    I just completed another challenge. 🤘🏽

    Feedback is always welcome. Please let me know of any issues you may find and/or how I can improve my code :)

    Happy coding!

    Galleria slideshow site

    1
    eagermonument81•210
    @ixtkPosted about 2 years ago

    Hey, looks like the images are being stretched. They lose aspect ratio. Avoid setting width directly, use max-width. Right now you can apply object-fit rule on the images. Look it up!

    0
  • eagermonument81•210
    @ixtkPosted about 3 years ago

    body::before creates horizontal scrollbar on big monitor screens. Also, it appears that if I hover buttons on left or right, it affects the rest of them by adding 1px border

    Marked as helpful
    1
  • eagermonument81•210
    @ixtkPosted over 3 years ago

    I wouldn't recommend using wv unit for font-sizes. The problem is that if user changes their font size in settings, your website doesn't adapt. It also doesn't change font size if user zooms in/out. So it is only responsive to width of the window, which is not ideal. On some widths, font looks way too big/small (743px, 764px)

    Other than that, consider adding main element and check the report

    Marked as helpful
    0
  • eagermonument81•210
    @ixtkPosted over 3 years ago
    • there's no h1 on the website, consider using it for "Group Chat for Everyone" text
    • .intro__title is not centered on desktop layout
    • there's horizontal scrollbar at about 800px
    • button text font size is smaller compared to the layout. Consider adding font-size: inherit
    • I think h2 font-sizes are too large for mobile layout
    • you could add main element in html to indicate which part of the page is the main content

    There are other layout issues like images not being centered on mobile screen or top images messing up layout. Make sure to use browser dev tools to inspect how different elements/layout will look on different screen sizes. If you have specific questions feel free to ask

    0
  • Chidelu•60
    @Karl-WilsonSubmitted over 3 years ago

    Please feedback on what to improve on will be appreciated. You can take a look at the project and advice me on what to improve on for instance view rendering issues, code structure, folder structure etc. Thanks

    Html, CSS, React

    1
    eagermonument81•210
    @ixtkPosted over 3 years ago

    The website says "loading". It doesn't seem to work

    0
  • Yuko-code•235
    @Yuko-codeSubmitted over 3 years ago

    This is my 8th FM challenge. I couldn't display the number thing (the ones in circles with vertical line on top) at the bottom properly.

    Meet Landing Page

    3
    eagermonument81•210
    @ixtkPosted over 3 years ago

    You can remove the padding and add display: grid and place-items: center to center the text in the circle. Give the bottom one white background color.

    There's horizontal scrollbar at about 1200px

    Marked as helpful
    0
  • eagermonument81•210
    @ixtkPosted over 3 years ago
    • display image as block. This gets rid of little space below it that looks like padding
    • research relative units, rem and em and why they're important
    • the purple bar appears in desktop layout too, set image width to 100% to make it stretch to the size of parent
    Marked as helpful
    1
  • eagermonument81•210
    @ixtkPosted over 3 years ago
    • you could probably use simple selectors instead of using classes, such as h3. I think classes come up when layout is more complicated and it would be pain to nest many many levels of selectors
    • I would use <p> for card bodies (semantic HTML!)
    • I'm not sure why 4 divs are displayed as grid
    • you didn't need to add another container (.cards) since .container does the same thing. You could have defined <div class="card-container">. Similarly, there are many unnecessary divs, for example, you didn't need to wrap button or icon image in a div
    • I don't think display: flex does anything for .card meaning you don't need it
    • there's no <a> in the button, so user wouldn't be redirected anywhere when clicking the button. I suggest using <a>, or if you want to use button for some reason, nest <a> inside to make it a link (you should probably only use <a> for purpose of links)
    • not as important, but some attribution text is not visible and there's some irrelevant text at the end ("adkjashdkjashdasj" :D)

    I think this one is little over-complicated. I suggest you think about in terms of little components when coding small sections instead of whole websites.

    Good luck!

    Marked as helpful
    1
  • eagermonument81•210
    @ixtkPosted over 3 years ago

    The logo text in the footer is supposed to be white; Using <p> for footer navigation links makes no sense, you can't be redirected to another page if this was multiple pages

    0
  • eagermonument81•210
    @ixtkPosted over 3 years ago
    • from about 1040px when scaling, the 2 images are stretching and losing aspect ratio. Consider using object-fit: cover
    • children in .bottom have no margin next to each other. The texts almost touch each other. Use row-gap or column-gap for space in between children if your layout is either flex or grid
    • you could have just used padding on img for those icons and not nest them in div
    • you could have displayed .bottom as grid the whole time. There's no need to display it as flex, just have either 2 or 1 column depending on the size of the screen.
    • the images that are snapped to the side aren't supposed to have border-radius the side they're touching browser edge on

    The way I did the snapping part is gave section container fixed padding, like 24px and then used margin-[left/right]: -24px on the image.

    Don't forget to check the report

    Marked as helpful
    0
  • eagermonument81•210
    @ixtkPosted over 3 years ago

    Looks good.

    I would go from 1 to 2 columns instead of 3 right away. You should really add alt texts for images as the report suggests.

    0
  • eagermonument81•210
    @ixtkPosted over 3 years ago
    • at about 800px, horizontal bar appears, avoid setting fixed width.
    • when resizing, the card stays the same width on both mobile and desktop and feels very fixed and not responsive
    • you need to capitalize "k" in "10k+ companies" stat section
    • displaying .info_content as flex accomplishes nothing, as it would look exactly the same width default display: block
    • instead of applying display: flex and margin-right: 3rem to .info_content, add column-gap: 3rem on parent .info.

    Don't forget to check the report

    1