Skip to content
Submitted over 2 years ago

Advice generator

Suprefuner•470
@Suprefuner
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Advice generator app
  • HTML
  • CSS
  • JS
  • API
2junior

Design comparison

SolutionDesign

Solution retrospective

Hi All,

Please check my solution here.

Community feedback

  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. You did a good job 😎

    keep improving your programming skills🛠️

    your solution looks great, however, if you want to improve it, you can fix these points: ✅ pay attention to the automatic report on your solution. you need to fix html and a11y errors

    ✅ You Need to Stop Targeting Tags in CSS. When you add CSS directly on tags, your markup can’t change. Your style is tightly coupled to your DOM, and any change increases the risk of breaking things.

    ✅ it's bad idea to rerender the whole card. You could update only quote number and text

    I hope my feedback will be helpful. You can mark it as useful if so 👍

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    1
    Suprefuner•470
    @SuprefunerPosted over 2 years ago

    Hi @isprutfromua,

    Thanks for your comment!

    1. fixed the html and a11y errors by changing the class container into a main tag and adding aria-label="Button to render new advice" to the <button>
    2. Well noted, I only target html and body in my CSS, or there is a better way to do?
    3. Fixed, instead rerendering the whole card, just rerender the data by using .textContent

    Again, thanks for your comment!

    0
    Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    @Suprefuner sorry, my mistake. I meant identifiers

     #btn-change {

    In everyday work, already set ID selectors require the use of inline styles and !important rules. But afterwards there are no more doors open for further refactoring.

    I'm really happy that I was useful for you =)

    Cheers ,peace and happy coding

    0
  • Clément Creusat•1,655
    @ccreusatPosted over 2 years ago

    Hi ! Good job there :)

    As @isprutfromua said, fix your html and a11y issues. Take care of his advices too.

    For the positioning on your component, you are using Flex on <body> with flex-direction:column. It lacks of centering because when using flex-direction:column, the "axis" changed. align-items: center is not positioning on the Y axis but on the X axis. So to fix your design, add justify-content: center; to center your component on the Y axis

    Hope it will help :)

    Enjoy coding!

    Marked as helpful
    0
    Suprefuner•470
    @SuprefunerPosted over 2 years ago

    Hi @ccreusat,

    Thanks for your comment!

    Since I was trying to do mobile first, I totally forgot to check how it looks on desktop mode. Fixed and thanks!

    0

Please log in to post a comment

Log in with GitHub

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord