Skip to content
OverviewSolutions
14
Comments
20
Joanna Skrzypczak
@joaskr

All comments

  • Joanna Skrzypczak•510
    @joaskr

    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
  • Joanna Skrzypczak•510
    @joaskr

    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 :)

  • Joanna Skrzypczak•510
    @joaskr

    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
  • Joanna Skrzypczak•510
    @joaskr

    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!

  • Joanna Skrzypczak•510
    @joaskr

    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
  • Joanna Skrzypczak•510
    @joaskr

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

  • Joanna Skrzypczak•510
    @joaskr

    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
  • Joanna Skrzypczak•510
    @joaskr

    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

  • Joanna Skrzypczak•510
    @joaskr

    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!

  • Joanna Skrzypczak•510
    @joaskr

    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
  • Joanna Skrzypczak•510
    @joaskr

    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

  • Leo Santillan•80
    @leocsdev
    What are you most proud of, and what would you do differently next time?

    If I recall it correctly, I just eyeballed the design. Not really looked into the design file. Which is I'm proud. But it's better to build the challenge with the design.

    What challenges did you encounter, and how did you overcome them?

    Not much since I have some background on how to convert the design to html/css

    What specific areas of your project would you like help with?

    JavaScript, diving deep into it.

    QR Code Component

    1
    Joanna Skrzypczak•510
    @joaskr

    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 <section class="container"> to <main class="container">

    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.

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

    Keep coding and good luck with future challenges!

    Marked as helpful
  • Joanna Skrzypczak•510
    @joaskr

    Hi :) Good job with recreating the design

    If you want to get rid of accessibility issues you should use the header tags in the correct order in your code - so h1 comes before h2, h2 before h3 and so on. Currently you have <h3> before <h2> and <h2> before <h4> hence the warnings.

    As for the HTML validation report - maybe you can consider using a border (top and bottom) and some padding to create these vertical separators instead of <hr>?

    I encountered a small bug with your solution - when I open the site on a mobile (for example iPhone SE) device and scroll down to the "Features" section the <p> text under the <h2> is barely visible or even invisible. You might want to look into that.

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

    Keep coding and good luck with future challenges

    Marked as helpful
  • Joanna Skrzypczak•510
    @joaskr

    Hi, It seems like there's something wrong with the image. You can try changing: <img src="/images/image-qr-code.png" alt=""> with <img src="./images/image-qr-code.png" alt="">

    As for the accessibility issues:

    • replace <div class="all"> with a <main> tag. It is generally best practice to use landmark HTML elements as they improve accessibility for users with screen readers.
    • don't leave alt="" text blank for images. This alt text makes your code more accessible and should be added.

    Hassia Issah already mentioned that but as per the responsive design best practice you should change px to rem/em. You can read an article about it here

    Let me know if you have any questions. Good luck with future challenges and keep coding :)

  • Joanna Skrzypczak•510
    @joaskr

    Hi :) Great job with the challenge! Your solution looks great! if you want to improve it further, here are some tips:

    Accessibility

    • Currently, all your elements are placed in a <div class="main-container">. In order to improve accessibility you should replace that with <mainclass="main-container">. It is a best practice to use landmark HTML tags such as <header> <nav> <main> <footer> because they improve accessibility for users with screen readers.
    • You don't use <h1> on your heading. It is important for accessibility to have a level-one heading that clearly describes the page's content. In your code <h2>Equilibrium #3429</h2> can be changed to h1

    Code

    • As per the Mozilla Developer a <article> tag should have a header. In your code, you are using an <article> as an image container and an author container. You can change that to <div> or <section>
    • In your solution, you are mostly using px. I would consider using rem or em as they are better for responsiveness. Helpful article on this topic

    Design

    • Your design looks almost flawless. I noticed only one issue - when you display your page on a mobile in landscape mode the content overflows and doesn't look good. You might want to look into that.

    That's all from me. Overall, great job!. Let me know if you have any questions.

    Keep coding and good luck with future challenges!

    Marked as helpful
  • Joanna Skrzypczak•510
    @joaskr

    Hi Paul, Good job! Your solution looks great and works without any issues 🎉 If you want to further improve your code you can implement the following things:

    Accessibility

    • You are wrapping the whole content inside a <section class="container">. You should replace it with <main> tag. It is best practice to use HTML landmark elements such as <header> <nav> <main> <footer> because they improve accessibility for users using screen readers.
    • You are not using <h1> element. It is also best practice to have a main header on the page (but only one!). You can change <h5>ADVICE #<span id="advice-id">117</span></h5> to <h1> and another accessibility issue will be resolved 😊
    • You left an empty alt tag for the image - you should always fill it because it makes it accessible for screen readers <img src="./images/pattern-divider-desktop.svg" alt="" class="divide">

    Design

    • You have a small issue with advice number - there are two # before the number (ADVICE ##145). It is because you have one # in your <h5>ADVICE #<span id="advice-id">117</span></h5> element and then you add another # with js adviceId.innerText = #${data.slip.id};. Removing one will solve the issue ✔️

    Code

    • You should remove all console logs if you don't need them anymore. There is one left on 11th line in index.js file

    That's all. Overall, great solution. Let me know if you have any questions.

    Keep coding and good luck with future challenges!

    Marked as helpful
  • Joanna Skrzypczak•510
    @joaskr

    Hi Aniket, Great job! Your solution is almost identical to the provided design and it works perfectly! I really like the animation on the button 🎲

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

    Accessibility

    • You should wrap your content in landmark HTML tags such as <header> <main> <nav> <footer> as they make your page more accessible and are especially useful for users with screen readers. You are currently using <section id="main"> -> this should be replaced with <main> element.

    Design

    • I would change cursor to pointer on the button. To make that you can add the following css code to the <button id=btn> element cursor: pointer;

    Code

    • I see that you are using rdm = Math.floor(Math.random() * 224) to get a random advice number. That step can be skipped completly as the API provides an endpoint for getting random quote. You just need to replace

    url = 'https://api.adviceslip.com/advice/' + rdm;

    with

    url = 'https://api.adviceslip.com/advice'

    You can see more details about it in the API documentation

    Overall, great job! Let me know if you have any questions :)

    Keep coding and good luck with future challenges

    Marked as helpful
  • Joanna Skrzypczak•510
    @joaskr

    Hi Yutika, Cograts on finishing the challenge! You did a great job - it looks almost identical to the design! I have a couple of suggestions - but just small ones:

    Accessibility

    • You are using <div class="qr"> as the main container for the qr code. You should consider changing it to the <main> tag. Generally use should use semantic HTML tags such as <header> <main> <footer> <nav> when you build your html, because they improve accessibility for people who are using screen readers
    • You used <h2> tag for the heading. Since this is the only and the most important header you should change it to <h1>

    Code quality

    • I see that you used margin to center the content on the page. I would recommend looking into flexbox because it makes easier to create flexible responsive layout. Here's a nice guide for flexbox

    That's all from me. I think that you did great!

    Keep coding and good luck with future challenges!

    Marked as helpful