Skip to content
OverviewSolutions
8
Comments
90
Joachim
@Thewatcher13

All comments

  • Joachim•840
    @Thewatcher13Posted over 1 year ago
    • Your perfume is not a heading, just a p tag
    • the old price shoukd have the s tag , the New just a span
    • use the picture element for desktop and mobile pictures
    • work mobile first
    • dont set a height on your container the content does that
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago
    • Dont set a height on the cardcontainer, the content provides the height.
    • Do not use absolute values for font size but relative rem values.
    • Your mobile version has no picture and the text is definitly not well aligned.
    • Use the picture element in html fir both picutres (desktop and mobile)
    • Dor the space between elements, on figma you can use the alt button, so click by example on the text in figma , hit alt and then move your mouse to the side that you want to know the space.. if you have figma
    • work mobile first, so design first the mobile version, then the tablet and after tablet the desktop
    • use rem instead of px for media queries
    • U should use an ul for the stats
    0
  • Willian Padilha•40
    @WilliaisSubmitted over 1 year ago

    I'm starting in css and what I felt the most difficulty with was centering the div.

    responsive page

    4
    Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi

    I've a few things to say:

    html

    • use a main landmark role
    • dont skip the heading order you cant have an h2 before h1
    • your alt on the img should be much more clear a qr code to where?

    css

    • There is no need to import a charset in your css..why did you do this?
    • dont use px for font sizes , but rem, look at the post from fedmentor called "why you should never use px for font size"
    • never set a height on a container the content provides the height
    Marked as helpful
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi

    Good that you used landmarks, but a few things:

    html

    • you're image should have a meaningfull alt text
    • dont skip the heading order, uou cant use an h2 before you used an h1
    • your section should be removed, make the main the container of your project.

    css

    • be consistent in your values, font size should be in rem
    • you dont need a media query here, but also, the media query is to big and should be rem instead of px
    Marked as helpful
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi Alek Can you take a look at my solution for the landing page? What do you think of my solution for the curved border?

    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi, I've read your code an have a few things that you can change:

    html

    • You should have a main landmark role in every project.
    • Don't skip the order of headings in html, you can't have an H2 before you have H1
    • do you know the meaning of strong? Strong isn't used for bold text, it says to screenreaders ("read this line with emphasis")
    • Apply headings to: your result/ great and summary
    • The 76/100 should be one p tag, if you're css breaks it seems now like:

    76

    /100

    with one p tag: 76/100

    • The right column shoud be an ul
    • Use landmarks in your html

    So your html is important for markup your website. Your html should be:

    • Readable without any line of css, think of it like a plain structured text
    • If a developer read your code without looking on your website, they should be able to know that is the main thing (h1) that is an ul,...

    css

    • Use a css reset, look on Andy's bell his website for a good and clear one
    • Never ever set font-sizes in px (absolute value), but in relative (rem) https://fedmentor.dev/posts/font-size-px/
    • give the h1 and p tag also a class, it is better to do this.
    • Your breakpoint should be much smaller then 1440px, something like 1024px or 768px (but convert to rem)
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi, I've analyze your code and note a few things.

    html

    • Your alt on the img is empty, be sure you have an alt for important pictures, not for decorative picutures. It provide accessilibility. Screenreaduers use it.
    • You should have a main landmark role in every project.

    css

    • Use a css reset, look on Andy's bell his website for a good and clear one

    • Never ever set font-sizes in px (absolute value), but in relative (rem) https://fedmentor.dev/posts/font-size-px/

    • give the h1 and p tag also a class, it is better to do this.

    • Don't set a height on an image

    • Try to create the design as much as possible, your img has no border-radius like on the design. Your solution has to much padding.

    I have written a solution with an explanation for beginners.

    I gave feedback to a few people and saw that they do the same things wrong, so I decide to write this solution. I think it could be really helpful for you.

    So you can read my solution with explanation here:

    https://www.frontendmentor.io/solutions/qrcode-solution-with-explanation-0LCmcOmbrj

    Marked as helpful
    1
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi,

    To answer your question if you look at your solution and the design..what do you think? I have analayze your code...

    html

    • Don't use br or hr
    • Don't use inline styles, it has the highest specificitty, use classes instead
    • your button should have a type attribute (type=button)
    • Use headings on your result/ greatsummary (don't skip the order of headings!)
    • The right column is an ul
    • Try to keep your code more readable

    css

    • Use a css reset, look on Andy's bell his website for a good and clear one
    • Never ever set font-sizes in percent, but in relative (rem) https://fedmentor.dev/posts/font-size-px/

    I advice you to do first the qr code challenge and learn more about html good resource: https://web.dev/learn/html/

    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi Bruno,

    html

    • The alt text for the image should be more clear; a qr code to where?
    • use a main landmark role in every project

    css

    • Use a css reset, look on Andy's bell his website for a good and clear one
    • Never ever set font-sizes in absolute values (px), but in relative (rem) https://fedmentor.dev/posts/font-size-px/
    Marked as helpful
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Hi,

    Good to see the approach of landmark roles in your html, a few things;

    html

    • You can never ever have more then one H1 heading
    • heading h3 is not correct placed, 76 of 100 should be one p, if I delete the css, the 76 is a seperate line, which makes no sense to the user
    • You should use an ul for the right section
    • Your button should have a type attribute (type=button) by default is is type submit
    • Be carefull with the use of section

    css

    • Use a css reset, look on Andy's bell his website for a good and clear one
    • Never ever set font-sizes in absolute values (px), but in relative (rem) https://fedmentor.dev/posts/font-size-px/
    • Work mobile first, use media queries for tablet and desktop design
    Marked as helpful
    1
  • Washdil•40
    @WashdilSubmitted over 1 year ago

    honestly i didn't found anything as difficult in this challenges it was quite an easy one

    Oder-Summary-Component

    2
    Joachim•840
    @Thewatcher13Posted over 1 year ago
    • Add landmark roles in your project
    • Your cancel should be a button, not a p tag
    • Your change has a p tag and an a tag, that's not good and right html. The p tag is not necessarily bcs the change is a label for the link..
    • Dont skip the heading order in html, you can't have an h2 before an h1
    0
  • Joachim•840
    @Thewatcher13Posted over 1 year ago

    Your alt text should be much more clear something like a qr code to frontendmentor Your class names too Don't skip the order of headings in html , you can't have an h2 before an h1... Add main landmark to your projects.

    note, please apply the feedback asap in your project, we give feedback for a reason. I Say this bcs I saw the feedback you get isn't applied to your project.

    Marked as helpful
    1