@llKryptonixll
P
Vladimir Damnjanovic
@dovlicioAll comments
- P@dovlicio
Hi, great work! Congratz
I suggest adding README.md and editing the template accordingly to describe your solution.
On mobile screen I found that for some reason spacing in the second testimonial card is not correct, also on mobile, quote icon should be hidden.
Marked as helpful - @maomaoXDWhat are you most proud of, and what would you do differently next time?
I used the BEM methodology and sass architecture.
What challenges did you encounter, and how did you overcome them?I used the mobile first approach it's easy to use this in the layout
What specific areas of your project would you like help with?I'll try to code this using react.
P@dovlicioGreat job! I can already see by your code that you have more experience than me in web dev. :)
Looking forward to learn more from your code, your design is responsive, looks good on all screen sizes.
I would only suggest fixing your README.md file on github as you should edit your readme.md and delete the template file.
Marked as helpful - @bulhakovolexiiWhat are you most proud of, and what would you do differently next time?
While working on this task, I didn’t encounter any significant difficulties. I decided to use the Sass preprocessor, just like in my previous project, to simplify styling with variables and mixins. However, this time, I moved all the additional code into a separate file, _system.scss.
What challenges did you encounter, and how did you overcome them?The only part where I had to refer to the documentation and seek help from Google was transitions. Now I’ll try to remember that the following code won’t work:
.links__item { a { transition: background-color color 0.2s ease-in; // For some reason, I thought I could list all the // properties to apply the animation to. } }The correct approach is:
What specific areas of your project would you like help with?.links__item { a { transition: background-color 0.2s ease-in, color 0.2s ease-in; } }I haven’t worked much with animations before, so they haven’t stuck in my memory very well. Hopefully, I’ll have more opportunities to practice in future projects.
P@dovlicioHi, great job! You did excelent work.
For transition property in your case you could also do something like this:
transition: all 0.2sMarked as helpful - @uptowngirl757P@dovlicio
Really interesting solution, I like it :) You did a great job, I never used sass or less yet, BEM convention tho I know, very good! Just one thing I have noticed according to style we got, we should have bold new price and for bigger screens maybe add some more padding around product content but all in all great job and a lot can be learned from your code, thank you!
- @SthamarP@dovlicio
Hi, congratz, good job:) Pay attention to font sizes for all screens and also there is some bug for smaller screens, your content is cutoff and have scroll, just a little fix and you're good :)
- @VikVas28P@dovlicio
Hi, I like transition effect on hover, totally forgot about it for my solution :) One thing I see is that your design breaks for smaller screens (mobile), so you should work on it and fix it, for bigger screens you did a great job.
- @givanimP@dovlicio
Hello, great job, I think the card looks great. Me personally, I like to change font size for different screens but that's just me :) Learning something from your code, maybe a little bit more semantic HTML and just to fix README.md, you should replace it with edited template. Great work, keep it up. :)
- P@CKHarrisonWhat are you most proud of, and what would you do differently next time?
I have been primarily working on back-end hardware facing development using C# and other technologies in my day job for the past two years, so I'm a bit rusty on my fullstack skills, especially my Front-End CSS development. I plan on using Front End Mentor as a way to sharpen my skills back up to the level they were at around two years ago. I used this challenge as a way to work on mobile-first development. Funnily enough, I encountered the same problem I always used to have, how to center the content in the middle of the page! Please let me know about anything you think that could be improved, I'm here to get better, so your feedback is welcome!
What challenges did you encounter, and how did you overcome them?Centering the content within the body, I once again had forgotten exactly how to do that, and had to do some research on how to do it. Centering content has always been my nemesis!
What specific areas of your project would you like help with?Like I have said previously, I'm coming back to Front End Development after a while and am a bit rusty. Any feedback on the problems with the CSS that you see, or any better ways to structure my CSS or content would be greatly appreciated.
P@dovlicioHi there, great work! A few things I would suggest: your card container should not be <main> because the <main> should contain the main content of the page. Since we are building just a card component, you could use <main><div class="card-container"></div></main>. That means your card component should be inside the main area of the page.
Also, you did a great job centering the items in the body—that's exactly how you do it! Setting the height to 100vh and aligning items horizontally and vertically using display: flex is the right approach.
One property that isn’t necessary is flex-direction because you only have one element in your <body>, which is the <main> element. :)
Marked as helpful
