Advice-generator-app

Community feedback

• 

  Danilo Blas•6,300

  @Sdann26Posted over 2 years ago

  Hi Gabriela!

  I'll give you some recommendations:

  • For the centering in this case you can better use display flex than using margins because it will not adapt to all type of screen vertically as well as in the result of the design. So we will modify these two elements: To the body give:

  body{
     min-height: 100vh;
     display: flex;
     flex-direction: column;
     justify-content: center;
     align-items: center;
  }
  

  With that it would already be more or less centered and now we modify the main like this: Remove: margin: 5% auto 5rem auto; Add: margin-bottom: 3em;

  With this the design will improve with respect to the centered one.

  • To eliminate the error regarding the button we will add the tag aria-label="Change to Advice ". We change from <button type="button" class="dice-btn">`` to <button type="button" class="dice-btn" label="Change to Advice"`. This error happens because usually the buttons usually have text but when they do not have text when a screen reader passes through them, it detects that the button does not have meaning and then with the aria-label we add that meaning for its reading.

  • Lastly if or if there must be a h1 in your project that is the main title of your current page you could change the <p class="advice-id">ADVICE #171</p> by <h1 class="advice-id">ADVICE #171</h1> and it would be solved.

  I hope my comments will help you to improve.

  Good Coding

  Marked as helpful

  1

  ---

  

  P

  Gabriela C•195

  @gcardenasdevPosted over 2 years ago

  @Sdann26 Hi Danilo! Thank you so much for your suggestions! I have implemented them and they work great!

  1

  

  Danilo Blas•6,300

  @Sdann26Posted over 2 years ago

  @gcardenasdev Hehehe, you can now generate a new screenshot of the design and in theory it should be positioned in the middle of the screenshot.

  Marked as helpful

  1