Skip to content
OverviewSolutions
23
Comments
72
Yemisrach
@Yemisrach15

All comments

  • Yemisrach•1,430
    @Yemisrach15Posted over 1 year ago

    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.

    0
  • Yemisrach•1,430
    @Yemisrach15Posted over 1 year ago

    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
    1
  • Noor Rahmi•250
    @rahmi1016Submitted over 1 year ago

    Submitted solution for the second time. This solution used html table for bar chart. The first solution (using chart.js) could be seen in https://github.com/rahmi1016/expenses-chart-component-main.

    expenses chart component with html table

    1
    Yemisrach•1,430
    @Yemisrach15Posted over 1 year ago

    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.
    1
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    0
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    1
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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.
    1
  • Ehiz Omokhomion•150
    @bakemonostanSubmitted about 2 years ago

    The filter functionality is not perfect yet. Getting the filter feature to work perfectly was a hassle

    job-lisiting-w-filter

    1
    Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    1
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    0
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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.
    1
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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.
    0
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    0
  • Yemisrach•1,430
    @Yemisrach15Posted about 2 years ago

    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
    0