Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions7Comments34

All comments

  • stephmunez• 435

    @stephmunez

    Submitted

    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

    #gsap#next#react#tailwind-css

    1

    eagermonument81• 210

    @ixtk

    Posted

    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

    @ixtk

    Posted

    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

    @ixtk

    Posted

    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

    @ixtk

    Posted

    • 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-Wilson

    Submitted

    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

    @ixtk

    Posted

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

    0
  • Yuko-code• 235

    @Yuko-code

    Submitted

    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

    @ixtk

    Posted

    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

    @ixtk

    Posted

    • 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

    @ixtk

    Posted

    • 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

    @ixtk

    Posted

    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

    @ixtk

    Posted

    • 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

    @ixtk

    Posted

    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

    @ixtk

    Posted

    • 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
  • Heritier Akilimali• 310

    @heritio

    Submitted

    This one was a challenge with vanilla javascript, would be easier to do with react, but i would like to hear fro you on how i would make this project better on file organisation and such, otherwise thanks for the feedback.

    Mobile first with bem and scss and javascript

    2

    eagermonument81• 210

    @ixtk

    Posted

    • 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

    1
  • eagermonument81• 210

    @ixtk

    Posted

    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)

    1
  • eagermonument81• 210

    @ixtk

    Posted

    • 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

    1
  • G Whitley• 90

    @gwhitdev

    Submitted

    I learned a lot about the Grid with this challenge. I think there's probably some efficiencies that I can make in the CSS, possibly with the actual grid layout itself.

    Responsive testimonials using CSS Grid

    2

    eagermonument81• 210

    @ixtk

    Posted

    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

    1
  • eagermonument81• 210

    @ixtk

    Posted

    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

    1
  • eagermonument81• 210

    @ixtk

    Posted

    • 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
    0
  • eagermonument81• 210

    @ixtk

    Posted

    • Avoid setting fixed widths/heights. Instead, let the content grow or shrink dinamically
    • Avoid setting fixed font-size, use rem, em or other relative unit for that
    • Use relative units like em and rem for paddings and margins around text. This will ensure that text has enough/appropriate breathing space when font size changes.
    • Don't use multiple h1s in one page
    • Sections should have h2-h6 tags to identify what the it's about
    • Look into semantic HTML topic, the h5 you've used should have been a p tag
    • Use meaningful class names, unlike para you've used

    Marked as helpful

    3
  • eagermonument81• 210

    @ixtk

    Posted

    When I set light mode, click some country and go back to main page, it reverts to dark mode. Also, don't forget to check the report

    Marked as helpful

    0
  • Karim• 590

    @Galielo-App

    Submitted

    The hero section, omg...

    Very hard! I was dealing with overflows, z-index, a lot of stuff that complicated everything lol! But that's it, I feel way more comfortable working with that weird stuff now.

    But overall, I liked the final result. What do you guys think?

    Mobile first, SCSS and Javascript landing page bundled with Vite.JS

    2

    eagermonument81• 210

    @ixtk

    Posted

    Very clean, I love it. Make sure to check the report and fix the issues though

    1
  • eagermonument81• 210

    @ixtk

    Posted

    There's no need to use grid here, you can make this particular component fully responsive with only flexbox.

    Few problems:

    • There's horizontal scrollbar on desktop layout
    • Component cuts off (top side) on mobile layout
    • You also needed to apply border-radius to the component

    Suggestions:

    • I would avoid setting fixed width and height with vw and vh units. Instead let the content grow or shrink the component and set margin for the sides on the parent elements. Maybe set max-width
    • I would use section HTML tag for this component
    • I would remove default margin from body

    Regarding purple background color:

    You could have added a separate div with purple background color, then used position property to position it on top of the image and give it little bit of opacity.

    Easier approach would be to use background-blend-mode property. Check that out

    PS: Check the website report

    0
  • eagermonument81• 210

    @ixtk

    Posted

    Flexbox and grid layout will make this easier to deal with. Definitely check them out.

    Marked as helpful

    0
  • eagermonument81• 210

    @ixtk

    Posted

    Looks good.

    • On small and medium mobile devices, horizontal bar appears
    • At about 900px, features grid items are not centered.
    • At about 1190px, there's very little space around features and companies sections
    • Make sure to check the report and try to fix the problems

    You can check out clamp() function in css for fluid font size

    0