Skip to content
OverviewSolutions
13
Comments
20
Dytoma
@Dytoma

All comments

  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge 👏

    I have some feedback for you:

    • First you can look at the Accessibility Report and the HTML validation report. You can find useful information about you code. When using the ul element make sure you use li elements as children and then you can wrap the content in an anchor tag. So your code might look something like this <ul> <li><a href="">Home</a></li> <li><a href="">New</a></li> <li><a href=""> Popular</a></li> <li><a href=""> Trending</a></li> <li><a href=""> Categories</a></li> </ul>

    • Secondly the hover effects provided by the design system are absent, you can implement that too to your code. You can read about hover effect on MDN docs hover📚

    Happy coding🙌

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge👏you've done a great job

    However I have some suggestions that you can use to improve your code:

    • Firstly when you submit a solution, you can have a look at the Accessibility Report and the HTML Validation Report that gives you useful information and some mistakes that you would want to solve. You can see in the reports that you use multiple IDs twice and as you know IDs have to be unique so you can change one of the duplicated ones.

    • Secondly I'll suggest you use classes instead of IDs. It helps you avoid multiple debugging problems and classes allows you to style multiple elements at a time. There's more reasons to that, you can read more about this on class vs id DEV article.

    • Lastly instead of using your <div id="banner-image"></div> empty, I suggest you append an img element to that div and then use object-fit property in CSS to style as you want.(<div class="banner-image"><img src="./assets/images/image-web-3-desktop.jpg" alt="Desktop image"/></div>).

    That's all for me, Happy coding🙌

  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge👏

    I have some feedback that you can use to improve your code:

    • First instead of using article elements, I will suggest you use div elements and give them a role attribute(eg: <div role="main">....</role>).
    • For the <div class="attribution">......</div> you can add a role="footer" you can learn more about landmarks here. And for <img src="./assets/images/illustration-thank-you.svg" alt="Illustration" class="article__illustration"> I will suggest you add an aria-hidden='true' attribute for screen readers to skip this image as it just serves for decoration.

    Happy coding🙌

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge👏 the animation on the advices looks amazing

    I have some suggestions you might want to implement to improve your code:

    • First you don't need the <header></header> and the <footer></footer> elements as they have empty content thus useless to your code and you could also put your h1 in the main element.
    • For your button, I suggest you give it an aria-label="new advice" and to the image an aria-hidden="true" as the image just serves for decoration. You cam learn more about ARIA.

    Happy coding👏

  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge👏

    However I thing you forgot to implement the javascript part for the carousel images and text. And your website is not responsive for mobile versions; you might want to look at that to. If your goal was to just code the layout of the page then congrats👏👏 I hope you will take some time to improve your solution.

    Happy coding

  • Dytoma•550
    @Dytoma

    Hey 👋

    Good job on completing this challenge👏

    If you want to have feedback, you should consider adding a repository for others to access your code. The link to your repository is showing a 404 error. Other than that your solution is perfect, responsive for all devices. Congratulations👏

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge👏

    Here are some suggestions you can use to improve your solution.

    • As you can see your footer element is overlapping with the page's content. So, to avoid that you can use a top: 0 instead of bottom: 0 for users to see who coded the website immediately when they visit the page or leave it as it is but it will be at the bottom of the page thus offscreen; at least it won't cause any overlapping problems.
    • Instead of the article element you can just section elements and then for the sections you can use article I think it'll be better.

    Happy coding🙌

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge, I have some suggestions that you can use to improve your code.

    • First instead of using a div to wrap the dice image, I'll suggest you use a button. The reason for that is that you want screen readers to notice users with disabilities to understand what you're passing through your page and then you can add an aria-label to your button and aria-hidden="true" to the dice image.
    • Here is the code: instead of this <div id="dice-neon-circle" onclick="getAdvice()"><img src="images/icon-dice.svg" alt="" srcset="" /></div> use this <button id="dice-neon-circle" onclick="getAdvice()"><img src="images/icon-dice.svg" alt="" srcset="" /></button>

    And you would want to add some descriptive content to the alt attribute.

    Happy coding🙌

  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge. Here are some suggestions you can implement to your code:

    • First the font provided in starting files is absent on your page. You can implement it using Google Fonts
    • For the social icons, as you used images you can provide a descriptive text to the ```alt`` attribute (ex: <a href=""><img src="images/icon-facebook.svg" alt="instagram"></a>) for accessibility.
    • For the mobile version you can add some padding to your div elements.

    That's what I wanted to highlight. Happy coding😁

  • Dytoma•550
    @Dytoma

    Hey 👋

    Good job on completing this challenge. I noticed that you didn't use the font provided by the designs and you can do so by using Google fonts and either link it to your html file or import it in your CSS.

    Happy coding🙌

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey 👋

    Good job on completing this challenge, your solution looks amazing👌

    However I do have one suggestion when I looked at your code:

    • Instead of using a <div class="line"></div> I suggest you use border-bottom as the div add more content to your page, it is better to use borders.

    Happy coding👏

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge

    For accessibility issues, I'll suggest you add the aria-label='next advice' attribute to your button and an aria-hidden="true" to the image inside the button as it just serves for decoration. You can read more about ARIA on mdn aria docs📚.

    Happy coding🙌

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey 👋

    Good job on completing this challenge

    For your questions, here is my suggestions:

    • To make your images responsive, you can give an object-fit: cover property to the image to fill in its container and you can give your images a display: block/inline-block to style it correctly and give it width: 100% and height: 100%
    • For easy way to write CSS code, I will suggest using CSS pre-processors such us SASS, you can check this Sass tutorial for more info
    • For javascript there are a bunch options out there but you should be comfortable with javascript syntax before you dive into learning javascript frameworks or libraries. You can learn React.

    Happy Coding 😇

  • Dytoma•550
    @Dytoma

    Hey 👋👋 Good job on completing this challenge. I like the layout of your solution.

    For accessibility, I have some suggestions you would want to add to your code.

    -For accessibility you'll have to add aria-label to your button as there is no text to describe it add an aria-hidden='true' to the svg inside the button.

    You can read more about ARIA on MDN ARIA Docs 📚.

    Happy Coding😇

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋 Good job on completing this challenge.

    However I do have some suggestions to improve your solution. -The dice is supposed to on Click get provide a new advice to the user so instead of using a div to wrap it I will suggest using a button and add some accessibility to it. Instead of <div class="green-circle"><i class="fa-solid fa-dice-five black"></i></div> I will suggest this code <button class="green-circle" aria-label="new advice"><i class="fa-solid fa-dice-five black" aria-hidden="true"></i></button>.

    You can read more about accessibility on MDN ARIA Docs 📚

    • You can also select this button element in your main.js file and add an event listener (click) and trigger the function that fetches the advice as a call back.

    Code: document.queryselector(button).addEventListener('click', () => getQuotes()

    Happy coding😇

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Your solution is amazing😍😍.

    Can you please tell me How you managed to get this wonderful animations😅.

  • Dytoma•550
    @Dytoma

    Good Job👏

    I couldn't access your code on github but you did well👌. And if you can include a README file as suggested in the starting files on your repository, it'll be very helpful for someone willing to see your work.

    Marked as helpful
  • Dytoma•550
    @Dytoma

    Hey👋

    Good job on completing this challenge, Here are some suggestions I have:

    • For accessibility when you have a button surrounding an image, you should use aria-label`` for screen readers and aria-hidden="true" ``` form the image. You should get something like <button class="open opened" aria-label="open"><img src="./assets/images/icon-menu.svg" alt="open" aria-hidden="true"/></button>

    You can read more about ARIA here 📚.

    Happy coding👏