Skip to content
OverviewSolutions
7
Comments
34
eagermonument81
@ixtk

All comments

  • eagermonument81•210
    @ixtk

    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!

  • eagermonument81•210
    @ixtk

    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
  • eagermonument81•210
    @ixtk

    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
  • eagermonument81•210
    @ixtk
    • 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

  • eagermonument81•210
    @ixtk

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

  • eagermonument81•210
    @ixtk

    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
  • eagermonument81•210
    @ixtk
    • 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
  • eagermonument81•210
    @ixtk
    • 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
  • eagermonument81•210
    @ixtk

    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

  • eagermonument81•210
    @ixtk
    • 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
  • eagermonument81•210
    @ixtk

    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.

  • eagermonument81•210
    @ixtk
    • 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

  • eagermonument81•210
    @ixtk
    • header needs some black overlay on top because I can barely see white navigation links. It was in design too
    • the modal popup isn't centered and is very long, maybe it would make sense to add a scrollbar
    • button font-size is very small in modal
    • I would add cursor: pointer to the modal close button and radio checkbox section

    Also, check the report

    When it comes to folder structure, this from pro membership challenge files has become my new template

    - assets
  - mobile
  - tablet
  - desktop
  - shared (for icons that appear on all layouts, maybe logo.svg)
- css
  - style.css
- js
  - main.js
.gitignore
index.html
    Marked as helpful
  • eagermonument81•210
    @ixtk

    Clean.

    I feel like full width button on tablet layout is overkill though. I would move it up sooner. (I don't know if it was in the design, just my opinion)

  • eagermonument81•210
    @ixtk
    • You can give flexbox children width: 50 or flex-basis: 50% to ensure they will only take half of the screen and won't span more than that.
    • At about 1000px the image/text sections on the top have big gap, they aren't connected
    • At about 800px images don't take full width and have whitespace around, fix this by applying width: 100% to image container
    • At about 1000px text gets too long and I would suggest using clamp() to not allow more than some number of characters

    These are just few issues, if you have specific questions feel free to ask

    Also, check the report

  • eagermonument81•210
    @ixtk

    Some notes

    • use row-gap, column-gap, or gap instead of grid-* since it has been deprecated. Now you can even apply gaps to flexbox layout
    • I don't think .testimonial needs display: grid since there's only one child inside
    • #three has incorrect grid-row value, it's supposed to start from row 1 and end at 3. You have it start at 2 and end at 3. This makes extra auto row appear. If you inspect with dev tools you'll see there's row-gap applied at the end of .grid. That's always hint that unwanted extra row got added and you need to fix the grid-row value of one of the children
    Marked as helpful
  • eagermonument81•210
    @ixtk

    Looks great!

    One little thing: header links have no color change hover state the way footer does (I don't remember if this was required)

    I like that you've used img tags for photography and design section. I used background-image and background-size: cover and sometimes the text overlapped with the fruit and I had to find sweet spot for position. It stays nice and clean in your solution!

    I would use section tag for testimonials section, and maybe the standout and transform divs could have been section too.

    Also, font-size doesn't scale if I change default browser settings which might get annoying for some users. I would use relative units there.

    Don't forget to check the report

  • eagermonument81•210
    @ixtk
    • definitely look into flexbox layout
    • search about box-sizing property
    • check the website report
    • use relative units like rem or em for font-size, and margin and padding around text. I've changed my default font-size in browser but the learn more button stayed the same size, which is not good for user experience
    • I would use cursor: pointer when hovering over learn more
    • I would use a tag instead of button tag for "learn more" because right now that button can't redirect me to any other page. It doesn't have the href attribute. You could put a inside the button but It's better to use a tag on its own for links