Any feedback would be greatly appreciated! I am ok with the carousel functionality on the mobile and tablet screens but the desktop version feels a bit off. I think with more images added, it would look cleaner but I wanted to stick with the challenge design and mockup.
Hello, Jose.
Nice solution.
Regarding the carousel, I do feel like there is a bit too much white space, and I see you are creating the effect by shifting the <ul> container over with each click. What if instead of shifting the <ul> you simply switched out images? This shouldn't require much except a little javascript and a loop.
I also see you have a couple of accessibility issues regarding your carousel buttons. I usually fix this by adding an aria-label, but there are other ways. Here is a nice article on accessible icon buttons
The site's responsive behavior is great so is the overall spacing.
Hope this was helpful. Wish you great success on your journey.
Please look at my work and provide me your feedback.
Thank you.
Wow. Nice job, Gaganpreet!
No accessibility issues, no HTMl issues, and from what I can tell at a glance, your CSS is well structured and organized.
The only thing that I can think of is maybe adding a README file to your repo. Frontend Mentor gives a good template you can use.
Best of luck to you, Gaganpreet.
I need suggestions if there are things I have did wrong and which best practices should I apply to make my code cleaner and presentable.
Hello Mark!
I think you did a great job.
I especially like how you made use of CSS custom properties. I'm really bad about that and always forget.
Something you might want to think about in the future is adding a bit more whitespace. I think appearance would improve by increasing .person's padding, grid-gaps, and line-height.
Also, roughly from screen size 385px - 510px there is side-scrolling. (Something I've also had issues with in the past)
I found that if you move the .container you have at the bottom wrapped in a media query, and instead place it in front of your previous .container declaration (ln 24).
While also moving that media query you now have at the top, to the second .container underneath and giving it a min-width of 825px. Works quite nicely.
Best of luck to you, Mark.
