@KINYENJE
Submitted
Looking to hire developers?
@Dytoma
@KINYENJE
Submitted
@Dytoma
Posted
Hey👋
Good job on completing this challenge 👏
I have some feedback for you:
First you can look at the Accessibility Report and the HTML validation report. You can find useful information about you code. When using the ul
element make sure you use li
elements as children and then you can wrap the content in an anchor tag. So your code might look something like this
<ul> <li><a href="">Home</a></li> <li><a href="">New</a></li> <li><a href=""> Popular</a></li> <li><a href=""> Trending</a></li> <li><a href=""> Categories</a></li> </ul>
Secondly the hover effects provided by the design system are absent, you can implement that too to your code. You can read about hover effect on MDN docs hover📚
Happy coding🙌
Marked as helpful
@Dytoma
Posted
Hey👋
Good job on completing this challenge👏you've done a great job
However I have some suggestions that you can use to improve your code:
Firstly when you submit a solution, you can have a look at the Accessibility Report and the HTML Validation Report that gives you useful information and some mistakes that you would want to solve. You can see in the reports that you use multiple IDs twice and as you know IDs have to be unique so you can change one of the duplicated ones.
Secondly I'll suggest you use classes instead of IDs. It helps you avoid multiple debugging problems and classes allows you to style multiple elements at a time. There's more reasons to that, you can read more about this on class vs id DEV article.
Lastly instead of using your <div id="banner-image"></div>
empty, I suggest you append an img
element to that div
and then use object-fit
property in CSS
to style as you want.(<div class="banner-image"><img src="./assets/images/image-web-3-desktop.jpg" alt="Desktop image"/></div>
).
That's all for me, Happy coding🙌
@Nahuel-cell
Submitted
I would really appreciate if you could give me feedback on my little project. The most difficult part of this was when I wanted to select another classification and make the orange color of the number go to the new element that the user clicked on. I don't know if I explained myself but I appreciate that you comment on my mistakes or improvements. Just judge for yourself and then tell me.
@Dytoma
Posted
Hey👋
Good job on completing this challenge👏
I have some feedback that you can use to improve your code:
article
elements, I will suggest you use div
elements and give them a role
attribute(eg: <div role="main">....</role>
).<div class="attribution">......</div>
you can add a role="footer"
you can learn more about landmarks here. And for <img src="./assets/images/illustration-thank-you.svg" alt="Illustration" class="article__illustration">
I will suggest you add an aria-hidden='true'
attribute for screen readers to skip this image as it just serves for decoration.Happy coding🙌
Marked as helpful
@SantiagoPonce
Submitted
@Dytoma
Posted
Hey👋
Good job on completing this challenge👏 the animation on the advices looks amazing
I have some suggestions you might want to implement to improve your code:
<header></header>
and the <footer></footer>
elements as they have empty content thus useless to your code and you could also put your h1
in the main
element.aria-label="new advice"
and to the image an aria-hidden="true"
as the image just serves for decoration. You cam learn more about ARIA.Happy coding👏
@Alexxis0
Submitted
@Dytoma
Posted
Hey👋
Good job on completing this challenge👏
However I thing you forgot to implement the javascript
part for the carousel images and text. And your website is not responsive for mobile versions; you might want to look at that to. If your goal was to just code the layout of the page then congrats👏👏 I hope you will take some time to improve your solution.
Happy coding
@ralphvirtucio
Submitted
What is your feedback?
@Dytoma
Posted
Hey 👋
Good job on completing this challenge👏
If you want to have feedback, you should consider adding a repository
for others to access your code. The link to your repository is showing a 404
error. Other than that your solution is perfect, responsive for all devices. Congratulations👏
Marked as helpful
@AnouerMokrane
Submitted
hello , this is my solution for news homepage template , any advices can make it better is appreciate.
@Dytoma
Posted
Hey👋
Good job on completing this challenge👏
Here are some suggestions you can use to improve your solution.
footer
element is overlapping with the page's content. So, to avoid that you can use a top: 0
instead of bottom: 0
for users to see who coded the website immediately when they visit the page or leave it as it is but it will be at the bottom of the page thus offscreen; at least it won't cause any overlapping problems.article
element you can just section
elements and then for the sections you can use article
I think it'll be better.Happy coding🙌
Marked as helpful
@Rooneyfull
Submitted
@Dytoma
Posted
Hey👋
Good job on completing this challenge, I have some suggestions that you can use to improve your code.
div
to wrap the dice image, I'll suggest you use a button
. The reason for that is that you want screen readers to notice users with disabilities to understand what you're passing through your page and then you can add an aria-label
to your button and aria-hidden="true"
to the dice image.<div id="dice-neon-circle" onclick="getAdvice()"><img src="images/icon-dice.svg" alt="" srcset="" /></div>
use this <button id="dice-neon-circle" onclick="getAdvice()"><img src="images/icon-dice.svg" alt="" srcset="" /></button>
And you would want to add some descriptive content to the alt
attribute.
Happy coding🙌
@Taiwola
Submitted
feedback needed
@Dytoma
Posted
Hey👋
Good job on completing this challenge. Here are some suggestions you can implement to your code:
<a href=""><img src="images/icon-facebook.svg" alt="instagram"></a>
) for accessibility.padding
to your div
elements.That's what I wanted to highlight. Happy coding😁
@Dytoma
Posted
Hey 👋
Good job on completing this challenge. I noticed that you didn't use the font provided by the designs and you can do so by using Google fonts and either link it to your html
file or import it in your CSS
.
Happy coding🙌
Marked as helpful
@Bakhriev
Submitted
waiting for comments
@Dytoma
Posted
Hey 👋
Good job on completing this challenge, your solution looks amazing👌
However I do have one suggestion when I looked at your code:
<div class="line"></div>
I suggest you use border-bottom
as the div add more content to your page, it is better to use borders
.Happy coding👏
Marked as helpful
@Lukasz-Milde
Submitted
Feel free to point out all my mistakes.
@Dytoma
Posted
Hey👋
Good job on completing this challenge
For accessibility issues, I'll suggest you add the aria-label='next advice'
attribute to your button and an aria-hidden="true"
to the image inside the button as it just serves for decoration. You can read more about ARIA on mdn aria docs📚.
Happy coding🙌
Marked as helpful
@ralphvirtucio
Submitted
The difficulties I found on building this projects are: implementing the curve background svg on the hero section and implementing the invalid text
I'm unsure on how I implemented the background svg and invalid text
What are the best approach for background svg and input fields active states?
@Dytoma
Posted
Hey 👋
Good job👏
It will be nice if you can include a Hamburger menu on mobile devices so that the navigation menu don't take too much space on small devices. You can check this tutorial for that.
Marked as helpful
@Eddie7145
Submitted
What I found difficult while building this project was most with the images, I had a tough time making them responsive, so I would like to ask my fellow developers for the best way to make images responsive (pinching them in and out).
The areas of my code I'm most unsure of are mostly my JavaScript section more especially because I'm new to it, but here it is below:
const CloseMenu = document.querySelector('.close-menu-sect'); const NavWrap = document.querySelector('.nav-wrapper');
CloseMenu.addEventListener('click', () => { NavWrap.classList.toggle('hide'); })
So my questions are :
@Dytoma
Posted
Hey 👋
Good job on completing this challenge
For your questions, here is my suggestions:
object-fit: cover
property to the image to fill in its container and you can give your images a display: block/inline-block
to style it correctly and give it width: 100%
and height: 100%
Happy Coding 😇
@3omarhassan1
Submitted
@Dytoma
Posted
Hey 👋👋 Good job on completing this challenge. I like the layout of your solution.
For accessibility, I have some suggestions you would want to add to your code.
-For accessibility you'll have to add aria-label
to your button as there is no text to describe it add an aria-hidden='true'
to the svg
inside the button.
You can read more about ARIA on MDN ARIA Docs 📚.
Happy Coding😇
Marked as helpful
@mostafa93sh
Submitted
@Dytoma
Posted
Hey👋 Good job on completing this challenge.
However I do have some suggestions to improve your solution.
-The dice is supposed to on Click get provide a new advice to the user so instead of using a div to wrap it I will suggest using a button and add some accessibility to it. Instead of <div class="green-circle"><i class="fa-solid fa-dice-five black"></i></div>
I will suggest this code <button class="green-circle" aria-label="new advice"><i class="fa-solid fa-dice-five black" aria-hidden="true"></i></button>
.
You can read more about accessibility on MDN ARIA Docs 📚
main.js
file and add an event listener (click)
and trigger the function that fetches the advice as a call back.Code: document.queryselector(button).addEventListener('click', () => getQuotes()
Happy coding😇
Marked as helpful
@ApplePieGiraffe
Submitted
Hey, everybody! 👋
I've given a lot of feedback on this particular challenge, and now it's my turn to give it a go! I hope I did okay! 😅
I ended up coding a lot more Sass and JS than I initially expected, but I learned a lot. I decided to put the slider images in my HTML so that I could use the <picture>
element for responsive images but stored the text in my JS (feedback on my JS would be appreciated).
I added some cool transitions to the mobile menu and navigation and created an intro animation (using GSAP) after stumbling upon this YouTube video and remembering Connor Z did the exact same thing (and blew me away when I saw it) when he completed this challenge! 😆
More importantly, I tried to ensure that the design scales up and down nicely (which gave me quite a headache because of this behavior of flexbox), but I think I managed to do it okay in the end.
Of course, feedback is welcome and appreciated! 😊
Happy coding, everyone! 😁
@Dytoma
Posted
Your solution is amazing😍😍.
Can you please tell me How you managed to get this wonderful animations😅.
@karthiksk9819
Submitted
@Dytoma
Posted
Hey👋
Good job on completing this challenge, Here are some suggestions I have:
aria-label`` for screen readers and
aria-hidden="true" ``` form the image. You should get something like <button class="open opened" aria-label="open"><img src="./assets/images/icon-menu.svg" alt="open" aria-hidden="true"/></button>
You can read more about ARIA here 📚.
Happy coding👏
@Pedro-S-Vicente
Submitted
All comments are welcome
@Dytoma
Posted
Hey, Good job on completing this challenge however I do have some recommendations for accessibility:
aria-label
attribute to your button as there is no text to describe the button and an aria-hidden="true"
to the image inside the button for screen readers to escape this element.
<button id="randomAdvice" aria-label="new advice"><img src="./images/icon-dice.svg" alt="Dice" srcset="" aria-hidden="true"></button>
.You can read more about ARIA
:
Marked as helpful
@danyczech
Submitted
Hi all,
in CSS I tried many different things for svg/img and span content to center them in their backgrounds (with background-color I made the background circle and the svg star and number written inside <span>1</span> weren't easy for me to center to the middle of this circle). Finally, I used display:flex & align-items and justify-content: center and I am not sure if this is the best practice or if the flexbox is too much for just centering.
Thanks for the advice!
@Dytoma
Posted
Hey👋 Congratulation on completing this challenge, I really like your solution.
Here are some improvements you can implement to your code for accessibility:
form
and inside of it, there should be five input radios
and each input should have a label
attached to it to make the buttons accessible. Finally wrap all the inputs and labels inside a fieldset
to prevent users from making more than one selection.You can read:
This is one of the recommendation I got on my solution that I think you can implement to your code too.
Good Job👏