@Forester04
Thomas Hertog
@thomashertogAll comments
- @thomashertog
This feels not completely finished to me. You're on your way there though. Overall, this solution is looking good! You've used accessible/semantic HTML where possible
A few things I've noticed
Design
- you're not using the full height of the window if it is bigger than your content
- there's no whitespace on the left your content when you look at it in the desktop view but slowly going narrower
- I saw an error image in your code but couldn't get to visualize it by using the page
HTML
- Your code is bloated with
<div>wrapper elements where I feel things could be much simpler, you don't need a wrapping element for your form items (e.g. input field and submit button) - You include both hero images (desktop and mobile) only to hide them with your CSS. Beware that this causes both images to be downloaded (and using data), you may want to look into the
<picture>element to resolve this - You have an
<h1>on the page (which is required for accessibility reasons), however I feel We're coming soon is not really helpful as a header. I'd suggest making the name of the company an<h1>(and hide it accessibly)
CSS
- There's some repeated statements here and there in your media queries. Be aware that if you have them in your normal CSS, they cascade over to your media queries (e.g.
flex-direction: columnon your body selector and again for your body selector inside a media query) - It feels a little awkward that you specified 3 rows for a grid but are using 4 rows, maybe you want to explicitly set their heights as well or (I'd do it this way I think) use sizing of the elements and let those determine the height of the rows which will be auto-sized if you eliminate the
grid-template-rowsaltogether - I feel there's a lack of structure in your CSS code, there is no grouping/ordering of properties that makes sense, selectors are thrown around and are varying a lot in specificity (read up on specificity graph to see if you can make some adjustments here)
- A lot of sizings are fixed (using absolute units like
px, you could benefit from converting those to relative units like%) - There's a lot of flexbox stuff going on where I feel you have done some overkill (probably as well because of all the wrappers)
Marked as helpful - @codercreativeWhat are you most proud of, and what would you do differently next time?
I am happy the way the app turned out.
What challenges did you encounter, and how did you overcome them?The positioning of the social media overlay was very challenging.
What specific areas of your project would you like help with?I am happy to hear ANY feedback for improvement. Thank you in advance!
@thomashertogYour card is way bigger than the intended design. Though no pixel-perfect match is required, this feels a bit too astray from the original challenge
Your HTML feels a little awkwardly structured. e.g. no <h1> present which I can understand since it's only a component and not a full page, however remember to include one if you're building full pages. You may also want to remove the alt text (and add aria-hidden) for the icons, since they are purely decorative (and properly labeled through their parent/container)
For future reference it may be easier/more practical to have your reset styles in a separate stylesheet you can also look into the
gap-property for the social icons instead of using margins on them Because of the way you implemented that share section (including a different button to get back), there is a slight difference in positioning of that button making it jump from one position to the other when toggled.I can't put my finger on it and say what exactly should be refactored, but I feel you've written a lot of CSS code where a simpler solution is available. Selectors feel more complex than they should as well. There are also some duplicate declarations (e.g.
display:flexwithin the.contact-sectionselector)Keep learning, keep improving!
Marked as helpful - @kendo-desu@thomashertog
Your HTML structure feels a bit weird because you made this challenge with flexbox. CSS Grid is far more suitable for this one. You may want to look into that. It would also make the
tes-grid-containerless confusing to use, since currently it's not a grid container but a flex container As for accessibility, you didn't include any semantics in your HTML. Jumped straight to heading level 4 (which is frowned upon). You wouldn't read a book that starts with1.1.1.1.instead of1.That said, this challenge doesn't have a visible single header (given you should only have one<h1>on a page at any time)Styling done with ID-selectors is also a very bad practice. IDs do have a very high specificity and should be avoided if possible (you can easily replace them with a class here)
Keep improving, keep learning :)
Marked as helpful - @PhilippeItsMeWhat are you most proud of, and what would you do differently next time?
The responsivness is quite cool.
What challenges did you encounter, and how did you overcome them?Little things to debog... chatpgt... w3school
What specific areas of your project would you like help with?I still need to check the solution to find you the path to take. I though of using the @media with différents grid-area but then I try to find a solution with the auto-fit approach and then went back to my first intuition... which was good.
@thomashertogI feel you have overdone accessibility a bit with this challenge. Imagine you were to look at this with a screenreader. Would you really believe that all the heading levels you used are appropriate? In my opinion this challenge only needs an <h1> to identify the page title and an <h2> to identify the section with the cards. The titles of the cards themselves feel a bit awkward as a heading to me. Same goes for the <h3> in the <hgroup> there is no added value in having that as a heading (to which you can directly navigate). On top of that it feels a bit awkward that you're structuring it like
- <h3>
- <h1>
more down the page 3. a collection of <h2> Heading levels should be thought of as a means to quickly navigate the structure of a website. I'm always comparing it to a table of contents. You wouldn't make a ToC with 1.1.1. above 1. which is above 1.1 up til 1.4, right?
<article> also feels a bit overkill for the different cardsabout the alt text for the icons. alt text should describe the image you're looking at, so i'd replace icon karma with lightbulb icon, though you may argue that these icons are purely decorative in which case i'd leave the alt text empty and add aria-hidden to hide it from screenreaders (as they have little value in a read-out page)
absolutely love how you made the effort of building an in-between layout with just 2 columns.
to make your CSS more maintainable/reusable/readable, you may want to look into CSS custom properties (some people like to call it variables) also, if you're writing with short-hand properties (e.g.
padding: 1rem 2rem 1rem 2rem), notice that there is a shorter version available which has the same effect ->padding: 1rem 2remhope this was insightful however you already did a pretty good job!
Marked as helpful - @ysstudio22What are you most proud of, and what would you do differently next time?
Applying different images for different view ports. I had to do a bit of research but it was worth it.
What challenges did you encounter, and how did you overcome them?The biggest challenge was eyeballing the card sizes for the mobile and desktop view. I went back and forth between the design, but the previous challenges provided the foundation for this current challenge.
What specific areas of your project would you like help with?I might try to tweak the spacing between the svg and "Add to Cart" text inside the button.
@thomashertogLooks pretty good. Found some elements with explicit height. You may want to remove that to reduce unexpected behavior.
Try avoiding the use of ID selectors as they are highly specific and are to be used very sparingly
Marked as helpful - @thomashertog@thomashertog
I didn't include an <h1> because this clearly is a component that should be included into another page (that will have the <h1>)
- @salahstack@thomashertog
The design is a bit too much off to consider this one finished in my opinion. Most of it is colors and sizing, so you're on your way there.
Other improvements I'd make on this
HTML/accessibility
- you have a lot of
<div>containers of which i'm not sure whether they're all needed/adding some possibilities - There's a
<header>and<footer>which is good but no<main> <section>without associated header is just the same as a meaningless<div>
CSS
- mobile-first vs desktop-first is implemented inconsistently
- a lot of
px-values which are better inem/rem - you clearly know about some of the (which i consider) more advanced features of CSS Grid, however, you seem to be applying them in the wrong place/manner which causes weird composition (e.g. your footer at about 890px wide)
Marked as helpful - you have a lot of
- @andresd0319@thomashertog
your solution looks good visually! some small code improvements can be made though
HTML/accessibility
- you've used a
<section>, however that doesn't add any semantics nor a landmark.<section>is only useful if you can associate it with a header <article>is something that should make sense on it's own page, so definitely not the right element for that plan<a>is for linking to another page, i'm a bit unsure what other page you would navigate to when changing a plan/proceeding, they might be better off as a<button>- you're missing an
<h1>, which is like reading a book without a titleµ - the image on top is meaningful content in my opinion, so it should be in the HTML with a proper
alttext instead of in CSS
CSS
- no particular comment here, though you may want to look into CSS custom properties (or variables as they are often called as well) to have a centralized place to edit those instead of having to edit them all over your code
- you've used a
- @rawan6602@thomashertog
Your solution is looking good visually! There are some few improvements to be made though.
HTML/accessibility
- you used a
<main>element which is good, however you also used<div class="paragraph">while there is a<p>element exactly for paragraph text - the results themselves (the elements with
class="record"in your code should be a list
CSS
- use
min-heightinstead ofheightto ensure overflow issues are not caused when content is expanding - try working with
em/reminstead ofpx
Marked as helpful - you used a
- P@gokhan-guneri@thomashertog
almost looking exactly like the design, so that's very good
few improvements can be made though
HTML/accessibility
- There's no landmark being used. Yes you used a
<section>however, that doesn't correspond with a landmark role. - You have a lot of
<div>wrapper elements which are (mostly) unnecessary
CSS
- there's a breakpoint for the smaller screen sizes but that one also has a minimum
- you'll be better off coding things mobile-first instead of desktop-first (which i feel you're doing now)
- a lot of
px-based values, which are totally unresponsive - you should use
min-heightas opposed toheightso elements get room to expand when needed instead of causing issues with overflow
- There's no landmark being used. Yes you used a
- @Uvejsii@thomashertog
like you said, it's not perfect yet, my suggestions to you
Design
- Have a look at the colors in the
style_guide.mdfrom the starter files, the ones you use, feel a bit darker on screen - the 2 sections at the bottom are not equally wide in your solution
HTML
- you're missing a landmark
- you're missing an
<h1>, remember headings should be in order and never skip a level
CSS
- there's a lot being done with
pxvalues, try to replace them withemorremfor better accessibility - I feel you have some rules duplicated in your code. Didn't find a particular example, but I saw it passing by
Marked as helpful - Have a look at the colors in the
- @Mygaverse@thomashertog
The visual is looking good. There are some improvements available on the code part though
HTML
- the headings are not hidden
- headings and their levels should be in order and still make sense (imagine stripping out all the other content, does it look like a sensible table of contents?)
- there should only be 1
<h1> - you have a
<div class="score-list">with<div class="list-item">, which is completely insemantic. Use a<ul>with<li>for this - the
<button>does not have atypeattribute
CSS
- you've included bootstrap, but i don't really see it being used (and it's making things harder to adjust)
- it's not responsive (not down to
375pxat least, like in the challenge requirements) - i've got a feeling that a lot of styling is already in the HTML which made it more complex than it should be, keeping your CSS smaller (which i don't necessarily think is a good thing either)
P.S. the border around the left column is coming from the
borderproperty in your.cardselector (probably coming from bootstrap) - @Ferperezm28
Product preview page built with html and css, with responsive design.
#accessibility#styled-components@thomashertogyour solution is not responsive... it's adapting to the width yes, but it still uses fixed sizing
other than that you're including both the mobile and desktop image at once (and hiding it via CSS) while you should use the
<picture>element to do that (and save bandwidth by only downloading 1 image asset)you used an
<h3>but you lack an<h1>and an<h2>, so your headings are out of order/incomplete - @kaushik-2318@thomashertog
I hate to break it to you but
this only looks like a very pale resemblance of the design
- colors are off
- sizing is incorrect
the code feels generated
- HTML is absolutely not semantic
- inline styles are being used
- positioning relatively WILL break if you make any change to contents
Marked as helpful - @anespoul34@thomashertog
Your solution is looking good visually, there are some improvements that can be made though.
HTML
- I feel you're using the heading levels wrong. If you'd write them out in a table of contents like this
1. <h1> / 1.1. <h2> / 1.1.1. <h3>it doesn't really make sense - The
<button>is missing atype - The content in the Why us section feels like a list of links to me, so I'm wondering why you marked it up as a
<p>with<br>
CSS
- no need to make the button fixed size in both
widthandheightusingpx - the code within the media query for your
bodyis not needed in a media query, it should be the default (and making sure that the mobile view is vertically centered as well) - a lot of the properties don't affect anything in particular, so you could easily save a few lines of code here and there (making it easier to change afterwards when necessary)
Marked as helpful - I feel you're using the heading levels wrong. If you'd write them out in a table of contents like this
- @hsrvms@thomashertog
the solution is almost looking like the design, yet there are some serious accessibility improvements to be made.
- you still have a lot of
pxvalues forwidth(and evenheight, which would be much better asmin-height) so it isn't responsive in any way - there is no semantic HTML being used (e.g.
<main>) - the rating is chosen from
<li>instead of using radio-buttons in a<form> - heading levels are not being used consistently (imagine them like the input for a table of contents)
Marked as helpful - you still have a lot of
- @eslamwaleed1@thomashertog
you may have completed the desktop design, but it is not responsive. I can't see the mobile view of this.
other than that, you have used absolute positioning for everything (with pixels) that is definitely not how layouting works nowadays. maybe you can try learning some things about flexbox and/or CSS grid to make a new attempt at this challenge.
your solution is tagged with #accessibility yet you still have
<div class="button">with an svg (for the icon) which is not hidden from assistive tech (as it should be since it's only decorative), and a paragraph for the text, but no<button>element is used...Marked as helpful - @Shohanojjaman@thomashertog
Visually very similar to the design so kudos for that.
It has some improvements that can be made though
- You used an
<h1>for the name of the product and an<h4>for the product type perfume. This is not how headings work (imagine reading a book and you have a table of contents going from1.to1.1.1.1.but the1.1.and the1.1.1.are missing). Don't use headings for styling, do that in your CSS - The product image is meaningful content so it should be in the HTML (not added as a
background-image) - The icon on the button is purely decorative so it should be added via CSS (with the use of pseudo elements) or hidden from assistive technology
- Using absolute positioning for the icon in the button feels really weird, though can be fixed with along with the previous one
- The button does not have a type attribute (
type=buttonwill do) - There's a typo in the name of your dark-cyan color (may seem small, but typo's will eventually haunt you later on when trying to reuse variables), also casing is inconsistent
- Font-size (as well as padding/margin) should be in
em/rem
Marked as helpful - You used an
