Advice generator using API, HTML, CSS and JS
Design comparison
Community feedback
- @robicode-05Posted over 1 year ago
Hi !
Well done for finishing this challenge ! I have some suggestions for you ^^
HTML
Keep an eye on the Accessibility report, on this page. But mainly, you should use semantic html tag, like the
main
to encapsulate your content.the heading element should decrease, in your case, the h3 and h2 are interverted.
JS
In the function
newAdvice()
, the url is being redeclare on every call.
Since this value won't ever change, it can be declared outside the function scope :(bonus tip: in Javascript, if the variable is never reassign, prefer const over let, it will let the web browser have a better optimization)
const url = "https://api.adviceslip.com/advice" async function newAdvice () { try { const response = await fetch(url); const data = await response.json(); showAdvice(data); } catch (error) { console.error(error); } }
the function
showAdvice()
is setting up the whole text "Advice #---", but Advice is a static text an never change overtime. So it's a good practice to include this text directly in the HTML, and use Javascript to update only what's necessaryex:
<h3> <span>ADVICE #</span> <span id="adviceNumber"> </span> </h3>
function showAdvice(data) { const adviceNumberShow = document.getElementById("adviceNumber"); adviceNumberShow.textContent = data.slip.id; const adviceTextShow = document.getElementById("adviceText"); adviceTextShow.textContent = `"${data.slip.advice}"`; }
In the js file there is this code :
x = window.screen.width; if (x <= 428) { document.getElementById('line').innerHTML = ....; document.getElementById('line').style.width = "295"; }
and this won't work. Since the x value is set on startup, it will never change if the page is resize. It will work on first load, but this is really not a good practice !
To handle multiple screen size, try to use CSS most of the time.
If you wan't to update content dynamically, you can use :
- the
picture
tag with embedded media queries : <picture>: The Picture element / MDN
exemple :
<picture> <source srcset="https://via.placeholder.com/1400" media="(min-width: 1400px)"/> <source srcset="https://via.placeholder.com/1200" media="(min-width: 1200px)"/> <source srcset="https://via.placeholder.com/800" media="(min-width: 800px)"/> <source srcset="https://via.placeholder.com/600" media="(min-width: 600px)"/> <img src="https://via.placeholder.com/400" alt="example"/> </picture>
- Or the css property
background-image
: background-image / MDN
If you REALLY REALLY need to use Javascript to react on resize,
I strongly recommande using a ResizeObserver. It will keep track of resize events, but with bettre performance than just adding a listener to rezize event.Otherwise congratulations on finishing this project, and have fun on the next one ;)
0 - the
Please log in to post a comment
Log in with GitHubJoin 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