Skip to content
OverviewSolutions
14
Comments
21
Joanna Skrzypczak
@joaskr

All comments

  • Dedeepya Reddy H•10
    @hdreddySubmitted almost 2 years ago

    What did you find difficult while building the project? Container Positioning Which areas of your code are you unsure of? I wanna get better at positioning containers exactly where I want.

    Newbie QR code challenge

    3
    Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi :)

    Good job with the challenge. You did great. If you want to further improve your solution, here are some tips:

    Accessibility

    • You should use HTML landmark elements such as <main> <header> <nav> <footer> because they improve accessibility. In your code, you can replace <div class="attribution"> with <main class="attribution">.
    • It is considered a good practice to use h1 on a page. In your case you can replace <div class="head">Improve your front-end skills by building projects</div> with a simple <h1 class="head">Improve...</h1>

    Code

    • I see that you used position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%); for placing the content in the middle of the screen. Even though it works, it is not the best and easiest way to do it. I would suggest looking into flexbox. If you would like to implement that in your code there are a couple of steps that you have to do:
    1. remove position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%); from <body>
    2. remove margin: auto; from <div class="attribution>
    3. Add the following properties to the <body>: min-height: 100vh; display: flex; justify-content: center; align-items: center. Flexbox just makes it easy to position elements and it's later easier to make it responsive.
    • You can also change px to rem/em. It is better for responsiveness to use these units. You can read more about them here

    Here are some useful resources for learning flexbox. Article explaining properties Short video and game to practice flexbox

    Overall, great job :) And congratulations on finishing your first challenge! Keep coding, don't give up and good luck with future challenges. Of course, feel free to ask me if you have any questions - here or on slack channel.

    Have a great day!

    Marked as helpful
    1
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi,

    good job with the challenge! Here are some small tips if you want to improve your solution:

    • If you want to get rid of the accessibility issue you can replace <p>Improve your...</p> with a <h1>. It is considered a good practice to use the main header on a page.
    • Because you are using margin: 10rem auto; your content is centred but it's causing a scroll on smaller screens because the content is too big for it - because of the margin. You can fix that and easily center the div with flexbox. To do that you have to:
    1. Remove margin: 10rem auto; from .container and remove margin: 0 auto; from .card
    2. In .container add the following properties: min-height: 100vh; /*sets the content height to the full size of the screen but will expand when content is bigger */ display: flex; justify-content: center; align-items: center

    Generally using flexbox or grid to center a div is considered to be the best way to center a div.

    Let me know if you have any questions and good luck with future challenges :)

    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi,

    great job with the challenge :) It looks great. If you want to further improve your solution here are some tips:

    Accessibility

    • Wrap your content in a <main> element instead of a <div class="container">. It is best practice to use HTML landmark elements such as <main> <header> <footer> and <nav> because they improve accessibility.
    • It is also best practice to have an <h1> on a page. In your case <h3> can be changed to <h1>. Generally, we should use headers in order - so we should start with <h1> then <h2> and then <h3>.

    Design

    • I noticed a small bug in your solution when you open the website on a mobile device in landscape mode. The content is too big for the screen and looks weird. You can fix it for example by changing:

    .container{ width: 100%; height: 90vh; }

    to

    .container{ width: 100%; min-height: 90vh; }

    Overall, great job.

    Keep coding and good luck with the next challenges!

    Marked as helpful
    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi!

    Great job with the solution. I really like the end result 🎉

    I would only suggest changing small 2 things:

    • Switch a <div class="attribution"> to a <footer> - it is good for the accessibility to wrap all elements on your page with HTML landmark elements such as <main> <header> <nav> <footer> because it improves accessibility.
    • Don't forget about adding an alt text to the images that describe what's in the picture. In your code you left that value empty <img class="image" src="./images/icon-sedans.svg" alt=""> You can simply use alt="sedan icon" and it will improve the accessibility.

    Overall, great job!

    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi Tushar Pandey,

    good job with the challenge! It looks great.

    If you want to make it even better here are some suggestions:

    Accessibility

    • Wrap your content in a <main> tag instead of using a <div> -> <main class="container"> Using HTML landmark elements, such as <header> <nav> <main> <footer> improves accessibility of your site.
    • You can also replace <h2> with <h1>. Since this is the only header in the solution it fits better to use a <h1>. Also, it is a good practice to have one level one header on a website.

    Design

    • I noticed a small bug when you open the website on a mobile device in landscape mode - the content is too big for the screen and is only partially visible. You may want to look into that. 😊

    Overall good job 🎉

    Marked as helpful
    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Great job! I really like your solution. It look perfect 😊

    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi Ítalo Gabriel, great job with this challenge! It looks awesome. I really like the loading spinner 😊

    There are a couple of things that you can improve - they are mostly related to accessibility:

    • Wrap your content in a <main> tag instead of using <div class="main">. Using landmark HTML elements such as <main> <header> <nav> <footer> is better for accessibility.
    • You are currently using a <h1> for a advice text and <h4> for advice number. I would consider using <h1> for the advice number because it is technically a header for the card content and <p> for the advice text. It feels more natural that way and generally we should use headers in order - so h1 then h2 then h3 and only then h4. You can read more about it here

    Let me know if you have any questions.

    Good luck with next challenges!

    Marked as helpful
    1
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi Tiff,

    Good job with this challenge!

    If you want to further improve your solution here are some tips:

    • To get rid of Buttons must have discernible text accessibility error you can use aria-label on the button. It won't change anything on the screen but will make the button more accessible for users with screen readers. <button aria-label="Get new advice" class="button">. Here you can see more ways to fix that.
    • Consider using rem and em instead of px for font sizes as they are better for responsiveness. Here you can read more about it.
    • I would also recommend removing console.log(data) from the 8th line in app.js file beacuse leaving console.logs are considered a bad practice.

    Overall, great job!

    Keep coding and good luck with future challenges

    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi, Great job 🎉 I really like your solution.

    I have only 2 small suggestions for you:

    • Add 'cursor: pointer;' to the button because it show the user that he can click this element
    • Replace Coded by Your Name Here. with your name 😉 Be proud of your solution

    Keep coding and good luck with future challenges!

    Marked as helpful
    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi,

    Good job with the solution! It looks just like the design! Here are some minor suggestions if you want to improve your code :)

    Accessibility

    • You should use HTML landmark elements such as <header><nav><main><footer> because they improve accessibility. In your code you can change <div class="container"> to <main class="container">
    • You should add alt text to <img> because it improves accessibility for users using screen readers.
    • There's no <h1> on the website - it's a good practice to include one. In your code <p class=headline> could be a <h1> element

    Visual aspect

    • I noticed a small bug in your solution. When you open the site on mobile in landscape mode the design gets a little bit broken. You might want to look into that.

    Code

    • You are using px in your project. As per the responsive design best practices you should use rem and em. Here's a nice blog post explaining them.
    • You are using position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%); to position the elements on the page. I would suggest looking into Flexbox as it makes positioning elements much easier. video tutorial article

    Overall, good job! Let me know if you have any questions

    Keep coding and good luck with future challenges!

    2
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi, Good job with the challenge. You did great :)

    2 small suggestions

    • change <div class="main"> to <main> as using HTML landmark elements (<main><header><nav><footer> is considered a best practice and improves accessibility.
    • also, change \ to / in <img> src as it's now causing HTML validation errors. (from this: src=img\icon-sedans.svg to this: src="./images/icon-sedans.svg")

    Keep coding and good luck with future challenges

    Marked as helpful
    0
  • Joanna Skrzypczak•530
    @joaskrPosted almost 2 years ago

    Hi, Congrats on completing the challenge!

    Here are some minor issues that you can fix:

    • Change <div class="parent"> to <main>. Using HTML landmark elements is better for accessibility so you should use elements such as <main> <nav> <header> <footer>.
    • The layout is broken on mobile devices smaller than 375px (width) - for example on Galaxy S8/S9. It is probably related to the media query screen and (min-width: 375px) and (max-width: 600px) as it applies the style only to widths between 375-600. Everything lower and higher than these values won't be affected by the styling.

    Let me know if you have any questions :)

    Keep coding and good luck with future challenges

    0