@Forester04Submitted 6 months ago
Latest solutions
Latest comments
- @thomashertogPosted 6 months ago
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 helpful0 - P@codercreativeSubmitted 9 months agoWhat 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!
@thomashertogPosted 9 months agoYour 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 helpful0 - @kendo-desuSubmitted 9 months ago@thomashertogPosted 9 months ago
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 helpful1 - P@PhilippeItsMeSubmitted 11 months agoWhat 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.
@thomashertogPosted 9 months agoI 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 helpful0 - @ysstudio22Submitted 9 months agoWhat 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.
@thomashertogPosted 9 months agoLooks 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 helpful1 - @thomashertogSubmitted about 1 year ago@thomashertogPosted about 1 year ago
I didn't include an <h1> because this clearly is a component that should be included into another page (that will have the <h1>)
0
