Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions17Comments35

All comments

  • Thomas Hertog• 715

    @thomashertog

    Posted

    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
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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 in em/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

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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 alt text 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
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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-height instead of height to ensure overflow issues are not caused when content is expanding
    • try working with em/rem instead of px

    Marked as helpful

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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-height as opposed to height so elements get room to expand when needed instead of causing issues with overflow
    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    like you said, it's not perfect yet, my suggestions to you

    Design

    • Have a look at the colors in the style_guide.md from 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 px values, try to replace them with em or rem for 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

    0
  • Mygaverse• 150

    @Mygaverse

    Submitted

    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!

    Results Summary Component Solution

    #bootstrap

    3

    Thomas Hertog• 715

    @thomashertog

    Posted

    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 a type attribute

    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 375px at 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 border property in your .card selector (probably coming from bootstrap)

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    your 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

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    I hate to break it to you but

    this only looks like a very pale resemblance of the design

    1. colors are off
    2. sizing is incorrect

    the code feels generated

    1. HTML is absolutely not semantic
    2. inline styles are being used
    3. positioning relatively WILL break if you make any change to contents

    Marked as helpful

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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 a type
    • 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 width and height using px
    • the code within the media query for your body is 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

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    the solution is almost looking like the design, yet there are some serious accessibility improvements to be made.

    • you still have a lot of px values for width (and even height, which would be much better as min-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

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    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 from 1. to 1.1.1.1. but the 1.1. and the 1.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=button will 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

    1
  • Joachim• 840

    @Thewatcher13

    Submitted

    This is a solution with a little more explanation for beginners like I was when i'm start my journey on frontendmentor.

    I'm still learning every day more and more and I think this kind of solution with a little bit of explanation could be helpfull to new people. I'm every day more familliar with the basics of html and css with thanks to frontendmentor!

    Please give feedback if I things say that not right or make sense.

    Product preview card - with explanation

    1

    Thomas Hertog• 715

    @thomashertog

    Posted

    visually very similar to design so kudos for that

    few small improvements can be made though

    • you choose a <span> in the button to have extra styling options available, yet you didn't make use of it (because all styling was already done on the button itself), you can remove the <span> since you don't need it
    • the button has no type attribute (type=button will do)
    • there is still a value in px for a gap somewhere in your CSS, which might not scale as well, you may want to replace this with em/rem

    Marked as helpful

    0
  • Danil_Dikhtyar 🔥• 180

    @Rock-n-Roll-CRC

    Submitted

    Hi there 👋, I’m Danil and this is my solution for this challenge. 🚀

    ⚡ Performance:

    • 90%-100% on lighthouse report and PageSpeed Insights. 🏆
    • W3C html validity checker (0 errors, 0 warnings). 🎉
    • Axe Dev Tools (Only color-contrast issue). ♿
    • Zooming checked. 🔎

    🔥 Features:

    • Responsive design. 📱
    • Near-perfect matching design. 🔥
    • Readable code with comments. 👨‍💻

    ❓ Help:

    • Is my website accessible? 💡
    • Is there any way I could add some more semantics to my HTML? 🚧
    • Is my CSS good? 📖

    🛠️ Built With:

    • Semantic HTML5 markup. ✔️
    • Flexbox. ✔️
    • CSS Grid. ✔️
    • Media queries. ✔️
    • Mobile-first workflow. ✔️

    👨‍💻 Languages Used:

    • HTML. ⚙️
    • CSS (Vanilla). 🎨

    Any suggestions on how I can improve and reduce unnecessary code are welcome!

    Thank you. 🔥

    Four card feature section

    #accessibility#lighthouse

    1

    Thomas Hertog• 715

    @thomashertog

    Posted

    This is already looking very good, as well visually like the design given as in the code.

    Some small improvements you could make however (in no particular order)

    • outside of a main element you don't use semantic elements (you could argue that the introduction at the top of the page is a <header>
    • it feels very weird to have a title at the top that is not a heading (and then the titles of the cards being <h2>), so I'd fix this by making your .introduction into a <header> with an <h2> for the .introduction-slogan and a <p> for the .introduction-description
    • the icons in the cards are added as background-images requiring you to write all sorts of positioning logic in the background position as well as need to accomodate extra padding in the HTML. I feel (but this is my personal opinion and don't know if it's necessarily better) this would be better if they were in the HTML (though hidden for assistive tech because they're clearly decorative and don't add any meaning).
    • working with grid-template-rows: repeat(4, 1fr) feels a bit squishy since now all your rows will have the same height. That would be bad if one of the cards contained longer content, you might be able to "fix" it with min-height on the cards
    • for a small project like this, it's less relevant, though you might improve on maintainability and/or readability in CSS by working with custom properties (especially for all the colors)
    • you could also have a border-top property on .card switching only the border-top-color on every card itself for better reusability (only need to change border-width in 1 place)
    • the design feels a bit weird around the breakpoint, personally (though i don't know if I did it in my own attempt) I'd add another breakpoint for a 2-column layout

    Overall, this is already a very nice attempt, keep going and improving

    Marked as helpful

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    looking very sleek.

    small improvements can be made though

    • there is no semantics present in your solution
    • you have an h2, but no h1, so that feels kind of weird (imagine reading a book where you have the chapters but it's missing a title)
    • height is extremely restrictive and might cause overflow issues, therefore it is better to use min-height

    keep going and improving though!

    Marked as helpful

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    while the site is looking almost like the design there are some subtleties that can be improved.

    Design

    • too much whitespace overall (i'm blaming the over-use of grid-template-rows)
    • the border-colors on the profile images is not necessarily white everywhere
    • the profile images are too big (in the design they're as big as the space needed for the next to it)

    HTML

    • your solution does not include semantic HTML (e.g. your .container element could be a <main>)
    • I feel like you used headings (<h1> to <h6> for styling). Styling should be done in CSS, heading levels should be in logical order. by that i mean: when you strip away everything else in your HTML, does it like a table of contents that makes sense?
    • the image you included for the quotation mark in the background should either not be in the HTML (and can be added via a pseudo element in CSS) or it should have the aria-hidden attribute to ensure it is removed from the accessibility tree
    • alt texts should have meaningful content (in my opinion: repeating the name is not meaningful, so those might be hidden as well, or you need to give them better alt texts)

    Marked as helpful

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    Solution is looking good (visually). There are some things to improve your code though.

    HTML

    • you have a <button type="submit"> but there is no <form>
    • the star image is decorative (at least in my opinion) so it should have aria-hidden attribute
    • the ratings should be an <input type="radio"> in order for the form to submit meaningful data. you can find more info here
    • you might want to work with a hidden attribute for the thank you container in order to remove it from your accessibility tree as well until the form is submitted

    CSS looks clean, couldn't find improvements there however, you may want to remove those commented lines as they distract you from the code that is needed

    Marked as helpful

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    • i feel the content in the "why us" section is more of a list than a paragraph, so i'd change that to a unordered list
    • you have a lot of duplicate CSS within your media query. bear in mind that everything outside of the media query still applies so there is no need to repeat it in there
    • i don't understand why you have a grid definition for your main element and yet still adjust sizing (in pixels for that matter) for each of the different sections

    there's a lot of good stuff going on as well, so keep up the good work!

    Marked as helpful

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    this is already looking good. few things you might want to address though. there are some accessibility issues in the report that are easily fixable.

    regarding your CSS code, you may want to look into reusing styles more. I've noticed you are using all different classnames. Know that classes are meant to be reused across various elements!

    1
  • A H M A D• 440

    @AhmaadAlharbi

    Submitted

    I just learnt css grid and wanted to pracrice on it

    Grid Css

    1

    Thomas Hertog• 715

    @thomashertog

    Posted

    you might want to define the structure of your grid on the parent as well by using template-columns/rows

    that being said, you can use grid just as well for the mobile layout. try thinking about how you should alter the grid definition to make sure the mobile layout fits in.

    0
  • Thomas Hertog• 715

    @thomashertog

    Posted

    there are a lot of issues in the report. Please try to fix them first!

    Marked as helpful

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    you might want to learn about how margins (and especially margin: auto) works in a flexbox context. this could easily replace the solution with position:absolute and make it cleaner. other than that, your solution is looking pretty darn good! don't forget to fix the accessibility issue as well

    Marked as helpful

    1
  • Thomas Hertog• 715

    @thomashertog

    Posted

    what @cyruskabir already said about the image. Other than that. think about your HTML structure. right now you have headings of level 2 (h2 elements) with the numbers of the stats. Imagine you're browsing the webpage with a screenreader without actually looking at the screen and tabbing through the headings. It probably feels a bit weird that you are just announcing the amounts (and i don't really know whether it would be useful to have these as header at all)

    0