Skip to content
OverviewSolutions
23
Comments
71
Yemisrach
@Yemisrach15

All comments

  • Yemisrach•1,430
    @Yemisrach15

    Hi Alfred, your solution is really good to be honest. You've used semantic HTML and the class names are also good. A few suggestions I can give you,

    • It needs some tweaking. Try to adjust the margins, padding, ...etc for different screen sizes so that it looks similar to the design. Use the mobile-first workflow.
    • For images, you should have an alternative text in the alt attribute. For instance, for the logo, it could be
    <img src="images/logo.svg" alt="Huddle Company" class="logo">

    or such descriptive phrase. If the image is for decoration only though, you don't need to have an alt text.

    • The .call-out can be improved like the below snippet.
    *html*
<div class="call--out">
    <h2 class="call-out-heading">ready to build your community</h2>
    <a href="#" class="btn btn--primary">get started for free</a>
</div>

*css*
.call-out {
    /* other styles you had */
    display: flex;
    flex-direction: column;
    align-items: center;
    justify-content: center;
}

.call-out-heading {
    /* other styles you had */
    text-align: center;
}

    Notice that I removed the .call-out__header div since it doesn't have any use and is only cluttering the document.

  • Yemisrach•1,430
    @Yemisrach15

    Hi Rashid, Well done! I like your solution. At the moment, the HTML semantics of the bar chart is not that good for accessibility. Using table is better in my opinion.

    PS. I used vercel and didn't have to configure anything. It detects your framework and bundler :)

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Noor, I like that you did this from scratch. A few suggestions,

    • A little work is needed on the responsiveness for smaller mobile views.
    • One h1 per page. You could wrap the spending - 7 days header with an h2.
  • Yemisrach•1,430
    @Yemisrach15

    Hi Felipe, responsiveness is well done. Few things I suggest you do,

    • You should include the overview section inside main
    • Change the text dark mode to light mode or vice versa when checkbox is toggled.
    • Input elements should have an associated label with discernible text for visually impaired users. Have a sr-only (screen reader only) text hidden from view but available to screen readers. The text could be something like Toggle mode.
    • The social media icons here have meanings and are not only for decoration so, they should have non-empty alt attributes that describe them.

    Congrats! No bugs in the method for dark mode toggle.

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hey David, solution looks pretty responsive :) A few suggestion I think would help you,

    • For the images inside the header, you can use the picture element to provide alternate images for difference device sizes. Here's a link for reference. Basically, what you have to do is as follow
    <picture>
  <source srcset="<link to desktop image>" media="(min-width: 768px)" />
  <img src="<link to mobile image>" />
</picture>
    • The remove (x) icon besides the tags should be a button since they're clickable. Buttons have the ability to be focused by keyboard users while the icon here is simply an image and there's no way of knowing whether it's interactive.
    • Same goes for the clear element/button and the tags in the cards.
    • For the alt attribute of images, you should provide a text that describes the image well for visually impaired users. So instead of a simple Company logo, you can be more specific and say, for instance, Photosnap's Logo.

    Hope these are helpful to you :)

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hey Nadia, hooray for the zero report! The sass architecture is useful as they say. I think it's more maintainable if you have a separate file for each component. I'm also trying to use 7-1 pattern. Here are a couple of suggestions I can give you,

    • Put the nav element outside main and inside a header element.
    • I wouldn't use article for the element with class .card. articles should have self-contained information but here the information is too large, in my opinion. For the blocks/boxes though it might be appropriate.
  • Yemisrach•1,430
    @Yemisrach15

    Hi Bakemonostan :) good job! Yeah, the filter has some bugs, work on it when you get the time. A few suggestion I believe will be helpful,

    • Create an intermediate design/layout for tablet devices. You have a lot of space to the left and right but you're not using it.
    • The div with class header_img is unnecessary. You could have the background image on the header element like this
    header {
  background-image: url(/static/media/bg-header-mobile.b7750a0c3e0c016763b9.svg);
  background-position: 50%;
  background-repeat: no-repeat;
  background-size: cover;
  height: 150px;
  object-fit: cover;
}
    • Don't use outline: none on elements as people using keyboard to tab through the page can't see which element is currently focused.
    • For heading elements, go in order of their level. Don't jump h2 & h3 and use h4.
    • I would put the tags (.roles) in a list within a ul.
    • Also, try preferring article, section, and other semantic HTML elements over div
  • Yemisrach•1,430
    @Yemisrach15

    Hey Zajac, good job! The report under your solution gives you a lot of information you can improve on. But here are a few of my suggestions,

    • If an image or icon/svg has a meaning within the context its used, it should have an alt attribute with text describing the image. So maybe you should use the inline svgs as images.
    • A lot of the text you put in a div should be in a p tag. div has no meaning/semantic and is usually used for decorating or layout purposes.
    • Overview - Today should be a heading in a h2

    Other than these the solution is nice but read up on semantic HTML elements.

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Aya, great job! A few suggestions I think would be helpful,

    • The 'x' buttons should have a discernible text meaning you should add an aria-label for screen readers.
    • The boxes could be article elements for better semantics.
    • The company logos alt attribute should be descriptive. 'company logo' does not fully describe the images. You could, for instance, say "Photosnap company's logo".
    • The tags in my opinion should be listed in a list, ul, element.
  • Yemisrach•1,430
    @Yemisrach15

    Hi Abdul, here are a few helpful suggestions I can give you, hopefully.

    • Use position: absolute when you want to take a content out of the normal flow. You're not using it correctly. I think you couldn't get the background image to cover the header so you resorted to this. Containing the hero as part of the header and adding the background image to the header instead of the hero should solve this for you.
    • The logo at the footer should be a link.
    • If you're providing an alt text to images, they should be descriptive so that visually impaired users can understand what the image is about.
    • For a/link elements that don't have text within them, you should add an aria-label attribute or an sr-only text, again for visually impaired users.
  • Yemisrach•1,430
    @Yemisrach15

    Hi Felipe, you did a good job with this challenge! A few suggestions I think might help,

    • input elements should be inside a form.
    • The #img-submit div should be a button as it's interactive. Or <input type="submit" />.
    • Instead of
    .header-wrapper > * {
  transform: translateY(+20%);
}

    you could add the following code to .header-wrapper__result since it's the only section that needs to be shifted up.

    .header-wrapper__result {
  position: relative;
  top: -40%; // adjust this
}

    I think this article might be helpful. Keep coding :)

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Amer, great job! This is how I approach dark/light theme switch too :) Here are a few suggestions of mine,

    • The theme toggle button is not accessible to keyboard users and other assistive technology users. I can't focus on the button when I tab through your page. So you need to change it to, for instance, a checkbox input, link the input to a label, style the label to look like the design and hide the default checkbox input. Clicking on the label is like clicking on the checkbox.
    • Wrap the section after the header with main tag.
    • Avoid adding unnecessary elements in the HTML as in the span the top horizontal bar on the cards. You can use pseudo elements as an alternative.
    • You should have only one h1 heading per page. It's what identifies the main title of your page so more than one title creates confusion.
    • The social media icons as well as the up and down icons should have an alt text as they contribute to the meaning of the cards' content.
    • The overview-today section is part of the main content of the page so it should be inside main. But you can put the .attribution in a footer element.

    Keep up the great work

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Marcos, well done! Your solution looks quite responsive :) A few suggestions,

    • Since the tags are interactive/clickable, they should be buttons. Maybe wrap the text inside each li with button.
    • After a tag is selected, it should be possible to remove it. If I want to deselect a tag, I should be able to remove that particular tag without clearing the whole selection.

    Keep up the good work

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Alexander, good job! A couple of suggestions,

    • You could use semantic HTML for accessibility instead of the divs. The cards could be article tags.
    • The top part with the heading and toggle button should be inside a header element outside main.
  • Yemisrach•1,430
    @Yemisrach15

    Hi Graham, good job but this needs a bit of work on responsiveness for tablet sized viewport. A few suggestions,

    • section element needs a heading.
    • The cards inside section imo should be article for better semantics. articles are used for self-contained information and it is appropriate to use them in this case.
    • input needs label for accessibility purpose as your report says.

    For your question, I prefer using dataset attributes on a container (eg. body). Inside the javascript code, I change this dataset's value based on user's preference and inside the CSS I do something like

    :root {
    <your css color variables for light mode>
}
[data-theme="dark"] {
    <your css color variables for dark mode>
}

    This is assuming a container such as body has a data-theme attribute. It's only my preference since it needs less js code :)

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Muhamad, you did a good job with this challenge. Brave has a built-in ad blocker which seems to be the reason why the api call is getting blocked. What I found from this stackoverflow answer is brave ad shields needs to be disabled in order for the api call to work. I have tried it and it works after disabling ad blocker :)

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Issac, great job! Three suggestions,

    • alt text of images should be descriptive.
    • Headings should be hierarchical. Go in order of their level h1, h2, ...
    • Never use px for font-size. Use em/rem so that it scales when zooming in the browser or if browser font preference is changed in settings.

    Happy coding!

    Marked as helpful
  • Yemisrach•1,430
    @Yemisrach15

    Hi Vishwajeet, your solution is responsive, great job! Here are some improvements I suggest,

    • The divs with class main and header can actually be replaced with their respective tags, <main></main> and <header></header>.
    • Headers should be hierarchical meaning you shouldn't skip h2 and use h4. They should go in order.
    • Only one h1 heading per document to indicate what the main page title is.
    • Provide descriptive alt text for images. This helps screen reader users visualize(?) the image and in case the image is not loaded the alt text is shown. This is for the social media icons in this case. For purely decorative images leave the alt empty.
    • Never use px for font-size. Use em/rem so that it scales when zooming in the browser or if browser font preference is changed in settings.

    Keep coding!