Dytoma
@DytomaAll comments
- @KINYENJESubmitted almost 2 years ago@DytomaPosted almost 2 years ago
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 useli
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 helpful0 -
- @maximusDecimalusMeridiusSubmitted almost 2 years ago@DytomaPosted almost 2 years ago
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 animg
element to thatdiv
and then useobject-fit
property inCSS
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🙌
1 -
- @Nahuel-cellSubmitted almost 2 years ago
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.
@DytomaPosted almost 2 years agoHey👋
Good job on completing this challenge👏
I have some feedback that you can use to improve your code:
- First instead of using
article
elements, I will suggest you usediv
elements and give them arole
attribute(eg:<div role="main">....</role>
). - For the
<div class="attribution">......</div>
you can add arole="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 anaria-hidden='true'
attribute for screen readers to skip this image as it just serves for decoration.
Happy coding🙌
Marked as helpful0 - First instead of using
- @SantiagoPonceSubmitted almost 2 years ago@DytomaPosted almost 2 years ago
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:
- First you don't need the
<header></header>
and the<footer></footer>
elements as they have empty content thus useless to your code and you could also put yourh1
in themain
element. - For your button, I suggest you give it an
aria-label="new advice"
and to the image anaria-hidden="true"
as the image just serves for decoration. You cam learn more about ARIA.
Happy coding👏
1 - First you don't need the
- @Alexxis0Submitted almost 2 years ago@DytomaPosted almost 2 years ago
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
0 - @ralphvirtucioSubmitted almost 2 years ago
What is your feedback?
@DytomaPosted almost 2 years agoHey 👋
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 a404
error. Other than that your solution is perfect, responsive for all devices. Congratulations👏Marked as helpful0 - @AnouerMokraneSubmitted almost 2 years ago
hello , this is my solution for news homepage template , any advices can make it better is appreciate.
@DytomaPosted almost 2 years agoHey👋
Good job on completing this challenge👏
Here are some suggestions you can use to improve your solution.
- As you can see your
footer
element is overlapping with the page's content. So, to avoid that you can use atop: 0
instead ofbottom: 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. - Instead of the
article
element you can justsection
elements and then for the sections you can usearticle
I think it'll be better.
Happy coding🙌
Marked as helpful0 - As you can see your
- @RooneyfullSubmitted almost 2 years ago@DytomaPosted almost 2 years ago
Hey👋
Good job on completing this challenge, I have some suggestions that you can use to improve your code.
- First instead of using a
div
to wrap the dice image, I'll suggest you use abutton
. 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 anaria-label
to your button andaria-hidden="true"
to the dice image. - Here is the code: instead of this
<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🙌
0 - First instead of using a
- @TaiwolaSubmitted almost 2 years ago
feedback needed
- help with the button behavior at the header, they seems to not occupy the whole space (more visible when using mobile)
- how to the image to cut from the left rather than the right
@DytomaPosted almost 2 years agoHey👋
Good job on completing this challenge. Here are some suggestions you can implement to your code:
- First the font provided in starting files is absent on your page. You can implement it using Google Fonts
- For the social icons, as you used images you can provide a descriptive text to the ```alt`` attribute (ex:
<a href=""><img src="images/icon-facebook.svg" alt="instagram"></a>
) for accessibility. - For the mobile version you can add some
padding
to yourdiv
elements.
That's what I wanted to highlight. Happy coding😁
0 - @AhmedGamalDev8Submitted almost 2 years ago@DytomaPosted almost 2 years ago
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 yourCSS
.Happy coding🙌
Marked as helpful0 - @BakhrievSubmitted almost 2 years ago
waiting for comments
@DytomaPosted almost 2 years agoHey 👋
Good job on completing this challenge, your solution looks amazing👌
However I do have one suggestion when I looked at your code:
- Instead of using a
<div class="line"></div>
I suggest you useborder-bottom
as the div add more content to your page, it is better to useborders
.
Happy coding👏
Marked as helpful0 - Instead of using a
- @Lukasz-MildeSubmitted almost 2 years ago
Feel free to point out all my mistakes.
@DytomaPosted almost 2 years agoHey👋
Good job on completing this challenge
For accessibility issues, I'll suggest you add the
aria-label='next advice'
attribute to your button and anaria-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 helpful1