Submitted over 2 years ago

Html sass and js advice generator

Rémy Boiré•350

@remyboire

Preview site View code

This is a solution for...Advice generator app

HTML
CSS
JS
API

2junior

Design comparison

SolutionDesign

Solution retrospective

Hi, my name is Rémy and this is my first challenge here !

As a junior programmer, I never tried to work with API yet. This challenge was a perfect first approach for me. I had trouble with fetch cache control because I wasn't aware about it. In my first version I made an setTimeout function who compared the last advice to the new one and loop while those to were similar. I implemented a loader to handle the waiting time. Adding { cache: 'no-cache' } remove the problem but I kept the loader just in case API doesn't respond fast enough. I also added a condition when the user click on the button to avoid queuing requests.

Thanks for taking the time to look at my project, any feedback are welcome :)

Community feedback

Chamu•13,130
@ChamuMutezvaPosted over 2 years ago
Hi. Congratulations on completing your first challenge. Here are a few items that you can have a look at:

a site should have a heading, especially the first heading element (h1) - that should be always be the first heading of your site.

the advice__dice is supposed to be a button. An anchor element is used for navigation to other pages or sections on a page, whereas in this instance this is more to do with click events for generating the quotes.

the site looks good on desktop , but on mobile(below 390px) , the divider does not look great. There is an overflow, check the impact caused by max-width: min(444px, 100% - 140px);
Marked as helpful
0
Rémy Boiré•350
@remyboirePosted over 2 years ago
Hi @ChamuMutezva, much thanks for your relevant feedback. I took a few minutes to consider it and make some changes. • I put "ADVICE #XXX" ad h1, not sure if it's the best option. Shall I have put the quote instead ? • I changed 'advice__dice' for a button as suggested. • I saw that problem but the pause icon were not centered anymore. I tried an other method that worked well, the svg is now a 'background-image' with 'position:center'. This way I don't need media query as I used only one version of the divider. How do you feel about this approach ? I'm really gratefull for you to take some time to digg in my code, thank you !
0
Chamu•13,130
@ChamuMutezvaPosted over 2 years ago
@remyboire , that's great, well done.

for the .container div , remove position: absolute; . Elements that are position with absolute can be tricky to control especially those following the element, however they will be occasions where you will definitely need position absolute . Also for the same .container div ,change min-height: 100%; to min-height: 100vh; - check the difference they make. A min-height: 100vh; will make sure that container fills the available height of the device , whilst 100% does not. Remove justify-items: center; as well - it is not doing anything

since the .advice__quote are quotes, take a look at the blockquote element - it is important to use semantic elements where possible . Read more here blockquote element - mdn

the h1 and the button looks good

well done
Marked as helpful
0
Rémy Boiré•350
@remyboirePosted over 2 years ago
Thank you for the clarification on position: absolute @ChamuMutezva ! Changes made ;)
0
Shashree Samuel•9,260
@shashreesamuelPosted over 2 years ago
Hey good job completing this challenge.

Keep up the good work

Your solution looks great however I think that your button needs a bit less padding and a bit of margin to the bottom.

In terms of your accessibility issues simply wrap all your content between main tags

I hope this helps

Cheers Happy coding 👍
1

Please log in to post a comment

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