@Forester04Submitted about 1 month ago
Thomas Hertog
@thomashertogAll comments
- @thomashertogPosted about 1 month 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 - @codercreativeSubmitted 5 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 5 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 5 months ago@thomashertogPosted 5 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 - @PhilippeItsMeSubmitted 7 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 5 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 5 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 5 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 10 months ago@thomashertogPosted 10 months 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 - @SalahEddineMjSubmitted over 1 year ago@thomashertogPosted over 1 year ago
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 helpful0 - you have a lot of
- @andresd0319Submitted over 1 year ago
I want to keep improving step by step.
@thomashertogPosted over 1 year agoyour 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
0 - you've used a
- @rawan6602Submitted over 1 year ago@thomashertogPosted over 1 year ago
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 helpful0 - you used a
- @gokhan-guneriSubmitted over 1 year ago
It was a nice work especially for reinforcing CSS Flexbox applications. I will be here for those who want to ask suggestions and questions. Enjoy your work...
@thomashertogPosted over 1 year agoalmost 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
0 - There's no landmark being used. Yes you used a
- @UvejsiiSubmitted over 1 year ago
I know it's not perfect so if you have any suggestions tell me, thank you in advance.
@thomashertogPosted over 1 year agolike 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 helpful0 - Have a look at the colors in the
- @MygaverseSubmitted over 1 year ago
I thought it was an easy challenge, but it took me quite a while to achieve the final result. There were some small issues with the layout and styles. I had to make several different approaches to the result. I did some reseaches and examples over the internet. I finally found a similar design that I could reference to. There are many techniques and skills that I can apply to my future projects. ps: I found there is a line of border around the left column. I haven't figure out where it went wrong. Let me know if you know how to fix it. Thanks!
@thomashertogPosted over 1 year agoThe 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)0
