@rvj1Submitted 4 months ago
Ryan O'Hanlon
@Ryan-OHanlonAll comments
- @Ryan-OHanlonPosted 4 months ago
This is an impressive use of only using CSS Flexbox and making breakpoints for both mobile and desktop. I would highly recommend taking the time to learn how to use CSS Grid as it can help you with placement of the container elements to be aligned.
Your code is structured well separating each testimony container in its own class but it can be improved as some testimonies are nested in each other in your HTML when they could be a child of the bigbox class and not the longbox class. Limiting yourself to just CSS Flexbox will only make placing complex elements like these more difficult in the future. Which I see with Kira's testimony being mis-shapen when I adjust the width of the browser that causes the margin and padding to fall when the display goes past 1000px.
Remember that when you need to place the entire body element in the center of the webpage to assign it the CSS attribute min-height with a value of 100vh.
You'll be using CSS Grid and Flexbox together for future projects to obtain that responsive web design. Here's a site that helped me out understand grid. https://gridbyexample.com/examples/ https://www.joshwcomeau.com/css/interactive-guide-to-grid/
Marked as helpful0 - @ShivangamSoniSubmitted almost 2 years ago@Ryan-OHanlonPosted 5 months ago
Overall, very nice job on this challenge. You've made it responsive, and you utilized both CSS Grid and Flexbox.
I'm having a bit of trouble understanding your HTML and CSS though. While you did structure your CSS well and showed me ways to use CSS that I never knew about using the style attribute in the DIV element. You could improve the HTML structure to make it more understandable using other HTML elements such as header and section instead of nesting multiple DIV elements and having to assign each one a class. Probably could find a way to streamline or reduce the amount of CSS yet achieve the same result.
The one thing I'm really impressed by was how responsive you made every element that I could shrink the size of the page down to 70px and every element is responsive.
0 - @DKVyotuSubmitted 5 months ago@Ryan-OHanlonPosted 5 months ago
Excellent job making the product page preview. It's an exact match down to the margin and padding of the text and button element.
It's also impressive that you made CSS rules for the width of the page and set the absolute width of the desktop image by pixels. There's a lot I could learn from this to make my projects have better responsive design.
I think the only thing I was surprised on was the choice of having the breakpoint for your @media rule for mobile be 600px. I thought the breakpoint was supposed to be set at 320 or 375px based on the styleguide.md.
You certainly have a better grasp at using flexbox and writing your CSS rules having each class inherit the previous using &. Very well done.
1 - @cassiopeia001Submitted 5 months ago@Ryan-OHanlonPosted 5 months ago
A very strong match to the desktop design. While the code is well structured, it could do better with added comments to understand what each section of HTML or CSS is doing what.
The color of Simple Omelette Recipe does not have any color attribute added from the style guide.
While the site is responsive to the desktop design, the webpage does not change to match the mobile design. You need to make a second set of CSS rules for the desktop using the @media rules and apply a min-width. Then put all the desktop rules to adjust the margin and padding for inside the @media rule.
/*Desktop CSS rules when wider than 375px*/ @media (min-width: 376px) { body{ } }Also very good using the margin left attribute to create space for all listed elements and the table.
0 - @cassiopeia001Submitted 5 months agoWhat challenges did you encounter, and how did you overcome them?
i had a problem with the jessica avatar image not loading when i deploy on github pages, solved it by changing the path from /assets/images/avatar-jessica.jpeg to assets/images/avatar-jessica.jpeg.
@Ryan-OHanlonPosted 5 months agoOverall, well done matching the solution cassiopeia.
The dimensions are a bit off but that is a nitpick that I can't really offer proper feedback as I'm still learning how to match the overall size of the challenge to the solution.
Two things I did notice though with improving your CSS. You added a hover rule to turn the social links to green but you didn't add a rule to change the color of the text to black.
.social:hover{ background-color: hsl(75, 94%, 57%); color: black;This will change the text to a black font when a user hovers over the buttons. Also we both need to change the pointer to black with a white outline.
Also, don't forget to change the style of .attribution to a white font and assign the href to your website.
Good job and keep up the good work.
0 - @chrisk71Submitted 5 months agoWhat are you most proud of, and what would you do differently next time?
This was a fun challenge! I incorporated CSS grid and flexbox in order to expand my skillset.
What challenges did you encounter, and how did you overcome them?I had a much easier time positioning elements in this challenge vs QR code challenge.
What specific areas of your project would you like help with?I'd like feedback on the HTML tags I used, as well as the CSS styling strings I used.
@Ryan-OHanlonPosted 5 months agoThis is a really nice solution that is an almost dead-on match with the design.
I wish I had some constructive feedback but it would only be nitpicks like how your solution is taller than the design. I couldn't even figure out how to match the font-weight of the text for "HTML & CSS foundations".
Solid solution and keep up the good work.
Marked as helpful0 - @Leeh27Submitted 6 months agoWhat are you most proud of, and what would you do differently next time?
Adicionei um visual diferente do recomendado pelo desafio, acredito que isso destaca mais.
What challenges did you encounter, and how did you overcome them?No inicio fiquei com dificuldades para começar, mas vi alguns exemplos e logo me habituei.
What specific areas of your project would you like help with?CSS e JavaScript
@Ryan-OHanlonPosted 5 months agoNot bad for a first attempt. This challenge is a lot more difficult when you know very little CSS like myself and my own design still needs improvement. But here is some feedback.
- Change the body element color to light gray as set in the style guide.
- Setting the body element to position absolute and using top, left to 50% and transform: Translate( -50%, -50%) will get the image and text in the center.
- Box that contains the image and text should be white
- Link the google font Outfit to the html using <link> in the head tag and the style in css or
0
