@arfath-aliSubmitted about 1 month ago
Max
@maxkdavisAll comments
- @maxkdavisPosted about 1 month ago
Really nice job with this challenge! Looks like you matching the design specs quite closely. A couple minor things I noticed, but some of these are more personal opinions/suggestions...
1.) Create custom variables not just for the color but typography too. For example, the <h1> element: --heading-line-height: 110%; --h1-font-size: 2.5rem; /40px/ --h1-font-weight: 900;
2.) Be intentful with how you name your element classes. I highly encourage you to dig into the BEM principle. It's a very common naming convention developers use for classes. For example, instead of naming your first image as '.main_img1' I did 'class="hero__img hero__img--mobile"'.
Hope this helps! And again, great job!
Marked as helpful0 - @tsiupaknazarSubmitted about 2 months ago@maxkdavisPosted about 2 months ago
- Amazing responsiveness for desktop, tablet, and mobile screens. Your final output mirrors the design very all. One small feedback is to give the entire grid a bit of breathing room from the viewport... your grid stretches the entirety of the screen, but the design has it centered. I recently found the 'place-content:center' css rule to be quite helpful in these scenarios.
1 - @JJorgeMS13Submitted 4 months agoWhat are you most proud of, and what would you do differently next time?
me encanto poder ocupar CSS grid, podriá cambiar la forma en que lo hago y encontrar mejores practicas.
What challenges did you encounter, and how did you overcome them?el desafio mas grande fue el acomodo de las tarjetas ya que no conozco casi CSS Grid, pero gracias a sus links de la pagina pude entenderlo mucho mejor.
What specific areas of your project would you like help with?Me encantría recibir feedback en como puedo mejor el acomodo de mis tarjetas con CSS Grid.
@maxkdavisPosted 4 months agoOverall your solution looks good and lines up well with the Design. A couple, minor suggestions:
-
Use Semantic HTML as much as possible. Instead of wrapping the header and description in a <div> element consider using the <header> element. I would also wrap the entire title in an <h1> and use the <span> for the bolded 'powered by technology' line.
-
Overall responsiveness is good! I noticed there was some misalignment in the heading when on a mobile breakpoint. Also, the final, desktop breakpoint doesn't occur until the full desktop breakpoint, which leads to a bit of an awkward layout between 400px and 1400px.
0 -
- @TheBeyonder616Submitted 4 months agoWhat are you most proud of, and what would you do differently next time?
getting the right size
What challenges did you encounter, and how did you overcome them?getting the right size for the card
What specific areas of your project would you like help with?Any advice would do
@maxkdavisPosted 4 months agoOverall, really nice work. I like your use of media queries to incorporate responsiveness in your design. The use of CSS variables is a nice practice too.
Some minor suggestions to consider:
- the content container's padding isn't aligned to the design, which is causing your h1 heading to not look like the designs.
- put all your right-side content into one container so that one padding property will align everything. Notice that your 'perfume' text is a bit left of the heading, and your button element is to the right of the pricing text. Ideally, this should have the same vertical left-side alignment.
- old price text should be smaller. consider the <small> text.
Hope these help! Again, nice job!
Marked as helpful1 - @gabriel-m-devSubmitted 5 months agoWhat challenges did you encounter, and how did you overcome them?
many. Especially the tables section. I even left this project unfinished because I couldn't match the boards to the original design. I did my best although I am not satisfied with the final result.
What specific areas of your project would you like help with?table section
@maxkdavisPosted 5 months agoOverall, you did an amazing job matching your final solution to the design specs. Could (minor) things I noticed:
-
For the <ul> element, you can adjust the size of the bullet point with the ::marker pseudo-class and 'font-size' property. So you can do something like
li::marker { font-size: 12px; } -
It doesn't appear your project is mobile responsive. You can read more into Media Queries or leverage a CSS Library (i.e., Bootstrap) to add this.
Nice job and good luck with your continued coding journey!
Marked as helpful0 -
- @savvycolleenSubmitted 5 months ago@maxkdavisPosted 5 months ago
Webpage looks great! I love your use of CSS Variables to more easily assign color to your elements. It looks like your solution is smaller than the design. I would encourage you to dig into Figma to get a better grasp on the various specs and sizes.
0 - @tickoxSubmitted 5 months ago@maxkdavisPosted 5 months ago
1.) Try to me more meaningful with the elements you're using to describe the content (semantic HTML). Some examples: • Experiment with the <button> element for the 'learning' text. • Be sure to add an alt attribute to your <img> for accessibility • Experiment with the <small> element to denote text of lesser importance (the published date text) • Main header should always be <h1>. Don't think about it as a sizing thing, but rather semantic and displaying to users (and SEO) which text fields are of most importance
Marked as helpful0 - @ankushPanaSubmitted 5 months ago@maxkdavisPosted 5 months ago
HTML & CSS
- Use the a header element (<h1>) to improve your html semantics.
- Size of the component can be changed using the max-width css class. This will help ensure the size of your component matches the design.
- Make your code more readable for other users. Removing spacing between lines 35 and 44.
0
