Subham Sharma• 130
@subhamengine
Submitted
This is my first JS project, I got to learn a lot. Please let me know your views on this.
Grover Richardson
@groverrichardson
All comments
@groverrichardson
Posted
Hey! Great job. On the desktop, everything looks pretty good and works properly. There are a few things I would change though.
First, the background color of the report header doesn't match the original design. The background for the "Report For" section. I would double-check to make sure that the hex code matches what was provided.
Also, most of the cards are a little taller than the initial design. It's a small catch, but it makes a difference in the overall design.
Lastly, and most importantly, the site is not responsive. Be sure to implement the mobile design. And make sure that the app is responsive for all screen sizes in between. I would suggest utilizing flex-box and flex-wrap to do a lot of the heavy lifting.
Hope this helps!
0- darryncodes• 6,430
@darryncodes
Submitted
Hi everyone 👋
I had a little fun with this one, as the advice is cached for 2 seconds I decided to add a loader and disable the button until some more advice was able to be fetched.
Any feedback that could help me to improve my understanding of working with APIs is very welcome. I'm thinking about adding some error handling.
Anyway until next time, happy coding 🤙
@groverrichardson
Posted
Great job man! The loading animation is a great touch. I would trim the timing of it to match up better with when the advice actually loads. I haven’t gone too far in the code, but my assumption the animation is hard coded or preset time-wise. The loading should stop when the page is fully loaded.
Also the dimensions of the card seem to be more square than intended. The original design dimensions are rectangular so I would go back and make sure the width and height match the design. But stay responsive.
Overall great job!
1 - Chirag Patel• 20
@chirag631
Submitted
@groverrichardson
Posted
Hey! Great attempt. However, visually some things seem a little off. The font-family is not correct and the main text is too large. Be sure to adhere to the font-sizes provided. It looks like maybe the full snippet from Google Fonts wasn’t pasted in your head.
Also, for images, be sure to include alt text. It’s a necessity for accessibility.
I also noticed that your file doesn’t have an JavaScript and all styling was done inline via Tailwind. I would recommend separating the html file out into separate styling and JavaScript files. That way it’s easier to read in the future.
But I didn’t see any javascript in the project at all. So the app doesn’t work and is currently static. I would double back to this and make sure you’re calling the API provided and updating the text from the response.
Marked as helpful
0 @Jank1510
Submitted
Hello frontend mentor community.
Here is my solution to the Advice generator app challenge.
I appreciate the comments.
@groverrichardson
Posted
Great job! However, the card looks a little wider on desktop than the specs call for. I recommend using a max-width similar to what’s provided. Also, I would recommend setting your “p-title” to an H1 for accessibility. You need at least one H1.
Marked as helpful
0- Rascaleuh• 20
@Rascaleuh
Submitted
@groverrichardson
Posted
Keep in mind the width of the card. It doesn’t seem like the width of the card respects the design. I would recommend a fixed max-width and then responsive for smaller screen sizes.
0 - Divyank Dubey• 60
@dubeydivyank
Submitted
@groverrichardson
Posted
The site isn’t completely responsive on mobile. I would check this again for smaller devices.
1
