Skip to content
OverviewSolutions
27
Comments
1296
Raymart Pamplona
@pikapikamart

All comments

  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, nice work on this one. The desktop layout looks good, however layout issues appears when resizing the browser from desktop view to mobile view as the layout is not responsive. Also, when you zoom out on your end, the components gets huge.

    Ivan already gave great feedback on this one, just going to add some as well:

    • Yep, when you code mobile first, resizing the browser using dev tools into a phone's size should always be done so that you'll really see the layout being shown into that size. Actually at first, I find it a bit confusing to do mobile first, but some challenges taken, i'm sure that you'll get the hang of it.
    • Never use position absolute a large container on your site, on your end, using it on the main tag. The reason for this is that, position: absolute removes the element from the flow, if the container doesn't have explicit sizes, it makes the container doesn't "capture" the child element. Try to inspect your site in dev tools, hover on the body tag, you'll notice that it doesn't have any size because its child element is being absolute. For this one, you can just remove all these stylings on the main tag:
    position
top
left
bottom
right
margin
width
height

    Then to properly center the content, add these first on the body tag:

      align-items: center;
  display: flex;
  justify-content: center;
  min-height: 100vh; # makes sure that it has sufficient height
  padding: # add some to prevent component touching the sides of the browser
  place-content: center;

    Then again on the main tag, you can just add:

      flex-basis: 100%;
  max-width: 500px; # convert to rem and change the size depends on the design;

    This makes the component more responsive. You can add min-height on the main as well if you want if you like a more consistent sizing.

    • Now, if you follow those above, you will notice that some or lots of element are being out of place because those elements are using the same stylings on the main. Again, remove all those stylings I mentioned earlier on each element and let the main container handle their positioning. If you need to place elements, don't always jump to position: absolute this should be the last case you want. Try searching or maybe looking up other website's submission as well to get some ideas when you are coding :>
    • Do not use id attributes to target an element on your css, using id creates problem due to specificity, always use class so that it could be more manageable and reusable.
    • As you may know, an h1 is needed for every webpage, the h1 describes the main content per each page. On this one, since there are no visible h1 on the page, the h1 will be instead a screen-reader only h1. Meaning, it won't be seen visually, but it is there. Have a look at this simple fiddle that I have about screen-reader text. Comments are already included, but if you have any queries, just let me know okay.
    • Instead of h4, use h2 on the "Advice" text. When you are using heading tags, make sure that they are on the proper level, when you use h4, make sure that h1, h2, h3 are all present before the h4.
    • When you need to make a text capitalized, you don't write them as it is like "ADVICE" on the html, this will make screen-reader read the word letter-by-letter and not by word. Use lowercase on them and just use text-transform: uppercase on the css.
    • If you want to align those items inside the main tag in the center, you can add:
    align-items: center;
display: flex;
flex-direction: column;

    On the main tag.

    • Remember when using img tag, always add an alt attribute. When you don't include it, screen-reader will instead announce something different from the file path. So always include it.
    • Since the divider img is just being used as decoration, adding alt="" and aria-hidden="true" on it would be nice. This makes the img tag be hidden for screen-readers as they are not really meaningful content of the site, always use this when image are not informative.
    • Instead of input, using button would be much better and correct for this one since input are used inside of form right.
    • Using button on that one, I would suggest the .circle selector to be the actual button, just style it to be circular. Also, since there are no visible text on that button ( if you used ), you should always add a label-text on it on what the button is supposed to do. For example, you can add aria-label="Change quote text". This way, when user traverses the button using screen-reader, they will be notified on what this button does.
    • If you are new to some of these ideas, maybe adding more will be confusing right now. But some ideas to make the app more accessible would be, shifting focus on the quote after the button is toggled or an alternative, aria-live would be use on the quote so that it will be easier to maintain response.

    Those might be lot but you'll encounter them on your way. Just let me know if you need help and see if I can help. Again, great job for this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, great work on this one. The desktop layout looks really nice, the site is responsive and the mobile state looks really nice as well.

    Others already gave their feedback which is nice to see, just going to add some suggestions as well and don't mind me if I re-iterate some ideas mentioned already:>

    • For the scss part, you don't really need to use:
    body {
  .... other selectors
}

    You can just directly target the selectors like .container, this way you reduce specificity. Also, if you like, you can search about bem convention. This will help you manage css selectors and create more generic classes. For example, you will have something like:

    .cards{
  &__container {}
  &__list{}
  &__item{}
}

    This way, you can group them properly if you think about it.

    • Adding max-width on the body tag to prevent the layout from stretching. If you try to zoom out on your browser, you'll see that the layout stretches, adding max-width will prevent that, just make sure to add margin: 0 auto so that the body will be centered.
    • These two text:
    Reliable, efficient delivery
Powered by Technology

    Are just one single phrase, meaning instead of using p tag for the first one, use a single h1 to wrap both lines and just add max-width on the h1 so that it will limit and create that 2 lines.

    • The overall font-sizes could be bigger, right now some text are small and they are smaller than the design as well.
    • Remember to only use a single h1 for a site. The h1 typically use on the hero heading like the 2 lines above on the site. So each of the card titles could be replace by just h2.
    • Each images could use an aria-hidden="true" attribute so that it will be totally hidden.

    Those only. Again, great job on this one.

  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, really nice work on this one. Overall, the layout for both desktop and mobile state looks really great. The reply also works great as well the upvote and downvote.

    For some suggestions, here are some:

    • If you try to click the first upvote of the first comment, you can't point to it , I think it is because of the h1 being sr-hidden. You can add clip: rect(0 0 0 0); on the h1 to fix it.
    • For each of the upvote and downvote, currently it doesn't have any informative text inside it and + or - alone is not enough. What you can do is that, add an sr-only text inside it to which could be downvote comment and upvote comment, depends on where it is placed.
    • Also, I would suggest changing the html placement for each of the post. The post's body should be placed before the ratings so that when a user navigates on the component, they will traverse the user, the user's post before the rating. Because it would be confusing to first traverse the upvotes/downvotes when the content is not first reviewed by the user.
    • Adding an aria-live attribute on the element wrapping the counts so that assistive tech will announce the update to the user on what happened. You can add extra text if you want, an sr-only text. Also, you could change the span to just p tag.
    • For each of the person's img, since their name is already present, it would great to use their name as the alt value since it makes sense to do so.
    • Actually, if this were a real app, each person would be a hyperlink right. Maybe making them wrapped inside a tag? Each person's img and name. Could be right.
    • Whenever you wrap a text, always use meaningful elements and not div. Changing all those div that wraps a text should be replaced by p tag.
    • For the reply button, the arrow-svg should have an aria-hidden="true" attribute on it since it is only a decorative image.
    • Again for the reply button, adding aria-expanded attribute would make it more accessible. This way, when a user toggles it, assistive tech will inform the user that it shows or expanding something.
    • For the reply form, using the user's name on the user's avatar alt attribute would be nice.
    • Adding label for the textarea would be great. Remember to always add label for each input or textarea element. This way, when a user is using a different language, those label text will be changed as well since attribute's data is static, the placeholder won't be translated. The label will be sr-only on this one.
    • There is a confusing part on the textarea. Initially, it should be empty right because a user is still not typing anything, but since you are already adding the @_name on field, a user can press the reply to send that reply even if they haven't add anything on the textarea and it will be much confusing since it is using required and to be honest, I can't think of an approach on this one, so maybe just letting you know about this? :>>
    • When clicking now either the reply or cancel button it would be nice again to have an aria-live element or maybe you can add a toast-notification on this one. The toast's text-content will vary on the user's choice, it could be like successfully replied to {person's name} or could be like reply cancelled. Just remember to add an aria-live="polite" on the element so that it will be announced.
    • When toggling the delete button, it would be nice to set the focus to the modal. This way, user will immediately be informed on what is the content. Right now, if you use keyboard to toggle the delete, then tab again, the user won't traversed on the modal like what it should do. So when a user toggles the delete, add the aria-expanded attribute then shift the focus in the modal, so that if they tab again, they will tab inside the modal. Another feature to add as well is to have a trap focus inside the modal. Meaning they can only tab on the two options, this way the user won't be confused on where they are at now.
    • To be honest, there are lots of added aria-live on this one or some toast, for the successfully deleting a reply and adding a comment as well.

    Just in general, if there are lots of state in your app that requires to send an update to the user, an aria-live would be nice or sometimes changing the focus on the pop-up element is really nice.

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, nice work on this one. For desktop layout, I think it looks fine, just needed for the proper font-weight to be used and also the svg's outline is overflowing, on the original, the outline should be cut. The mobile state looks fine as well.

    On your questions:

    1. Yep, working with images sometimes is hard specially when you need to position: absolute something but I think that you did great on this one!
    2. For the breakpoints, it depends sometimes on the design of the project that you are creating, sometimes it doesn't need to use breakpoints especially when you created the component really responsive. For me, when I use breakpoint, I typically go with 768px for the tablet and 1000px on desktop state, I always start now with mobile first approach. Sometimes this changes because like I said, it depends on the layout.
    3. You can if you want and there's nothing wrong with that. I use that when there is only a single component of the page and I want the whole body to occupy the full height.

    Here are some other suggestions for the site:

    • When including images on your site that acts only as decorative images, you should always use an empty value for the alt attribute and adding an extra aria-hidden="true" attribute on the img would be nice. On this one, the lady illustration should be using those attributes I mentioned since it is only decorative.
    • Just a remined, only use descriptive alt values for images that are really meaningful on the site and when adding the text, the text should not include words that relates to graphic such as illustration, image, icon since the img tag is already a graphic and no need to describe it as one.
    • Change those FAQ question from using h2 into using button or maybe just use a details element on this one. When creating applications or websites, if a component is interactive, always use interactive elements like button. I saw that you are using the div as a toggle for the content which should not be the case.
    • Those arrow-images should be hidden as well since it is only a decorative image. Use the above method I mentioned to hide it.
    • For each question's answer, those are only being hidden visually by the overflow and max-height but a user can still traverse those. If you need to hide an element, adding the visibility: hidden should be done and visibility: visible if it needs to be shown.

    Aside from those, great job again on this one. Let me know if you need any help.

  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, nice work on this one. For the layout, on desktop view maybe adjusting the padding would be nice. If you look at the design, there is a fixed padding on the left and right side for each of the landmark elements, you can follow that to improve the ui. Some text as well got smaller after the hero section. For mobile state, I think it looks fine.

    Here are some suggestions besides Divine Obeten feedback:

    • For the the site-logo-link, always remember to add either aria-label or an sr-only text inside it so that a user will know where the link would take them. You do this when there are no text-content inside the anchor. For this one, you can use like aria-label="homepage" on the a tag.
    • Always add the site name on the site's logo because that logo is one of the meaningful element on the site. Use alt="Huddle".
    • You can include the site-logo-link inside the nav if you want since it is being treated as link. But to be honest, I think you can just replace the nav by just a div since the link/s inside is not much, the nav would be much better on the links inside the footer tag.
    • Don't use height: 100vh on an element, this will make the element's height not consistent. Try going into dev tools at the bottom, you will notice that the hero section's height got small because of this property. Instead, use min-height: 100vh so that the container will respond properly.
    • I would change those section tags into just div because section alone is not that informative as a landmark element. By navigating using screen-reader, when it traverses the section tag, it won't announce it as a landmark even if you are traversing it as a landmark compared to like main, header and footer. div would be fine^^
    • Don't use br tag to cut the text, you can just add max-width on the text-element.
    • For the hero-image, you can add an aria-hidden="true" attribute on the img tag so that it will be completely hidden for other assistive tech. You can this as well on the svg's that are used across the page.

    FOOTER

    • On the logo, you are trying to make it white right, adding background-color to img won't work. What you can do is that add the svg's code itself on your html, then change the fill property of either the svg or path I think to the color that you want and this will changed the svg's color.

    • Same goes again for the logo-link, use a text that describes where the link would take and use the site's name as the alt value.

    • Those 6 links are all related to one another and using a single ul to wrap them would be better and also, you can wrap that ul tag by nav tag since those are the site's navigational links.

    • Those social media links could be wrapped inside a ul tag as well since they are list of links.

    • Since you are adding a hover state for the social media, you are implying that those are interactive, hence wrapping those inside their own a tag would be better, added as well the sr-only text or aria-label pointing on where it would take the user.

    • Lastly, if you pushed an update to your solution, clicking again the generate report so that it will clear up some issue if you fixed it.

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, nice work on this one. The layout for the desktop is fine but it could use more width since it is small right now. Resizing the site, it works fine but the layout at 1000px below until mobile state, the layout doesn't respond well making each content of the main squished and at 600px to 800px, you will noticed that the text on the left are now being overlapped by the form. The mobile state looks great though.

    Here are some suggestions for the site:

    • You don't use a height on the body tag. I'm supposing that you want the body to take the full height right, instead of using height, use min-height so that whenever the content of the site gets bigger, the body will rescale to that size because min-height lets the element expand unlike height that gives a fixed size.
    • Instead of using width for each child of main, you can just use padding and maybe add a max-width on the body tag so that it will prevent the layout to just always take full width of the user's screen. Just make sure to add margin: 0 auto for it to be centered.
    • Replacing each section tag into just using div. Using section is not really informative at all as landmark because it doesn't give extra information about it when traversing using an assistive tech, unless you give an aria-labelledby to it pointing to an heading's id. div is much better to wrap contents.
    • Whenever you use an input tag, adding a label pointing to it will be great so that even if the user uses a different languages, text content on the site can change if they want to translate the text and values used in attributes like the placeholder or maybe an aria-label are not translatable. Adding the label on this with an sr-only class will be great.
    • For the submit-button, instead of using input, use button type="submit" to be more explicit.

    SUBMITTING A WRONG FORM

    • If such input is invalid, adding an aria-invalid to that input would be really great so that when the user traverses on it, they will be notified that the input is wrong.
    • The error messages for each should have an id. I would change the div into just using p tag since it is a text content. Each error messages should have an id to which will be referenced by the input using the aria-describedBy attribute. This way, the user will know what kind of error that they had made because of the error message. Each id should be distinct like id="firstName-error".
    • Lastly, to further improve the error-handling, you could add an element that uses aria-live inside the form. The text will vary according to the form's submission, it could either be something like form has been submitted, thank you or it could be form submission invalid, please check your first name, last name... inputs.

    Here is a sample block that uses the attributes to add each error:

    if ( input is invalid ) {
   input.setAttribute("aria-invalid", "true");
   input.setAttribute("aria-describedBy", id of the error message");
} else {
   input.removeAttribute("aria-invalid");
  input.removeAttribute("aria-describedBy");
}

    Here is a sample form submission of what I said. It is a simple accessible form that I created, the aria-live is implemented there as well. Let me know if you have questions about it^^

    • Lastly, addressing the responsiveness issue of the site. Try to check out the dimension that I said earlier and try to fix the responsiveness.

    Still, great work on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, nice work on this one. I see that you somehow went on another approach on this one, it's fine but I really recommend that when you tackle other challenges, following the design itself should be the best ui approach :>

    Here are some other suggestions besides Deniel helpful suggestion and don't mind me reiterating some already suggested ones:

    • Using main tag on the .container instead of div would make the site more accessible. When creating websites, you should use main tag to nest the main-content of that webpage, on this case, the whole content should be inside the main.
    • To center the contents on your solution, you don't need to use margin on the .container since it is not really consistent. What you can do is that, add these stylings on the .container:
    justify-content: center;
min-height: 100vh;

    This way, the item will be centered properly and dynamically.

    • The margin on the .card could be removed since its parent ( .container) is already making it centered.
    • For the img, you could just use alt="" and aria-hidden="true" on it since the image doesn't really depict anything visually and could just be left empty so that screen reader won't read it.
    • Also when you use img or svg , you don't use words that relates to graphic like "image"` since those elements are already graphics.
    • For the bold text, you can use h1 on it for now. Remember that a webpage needs to have an h1 and you can replace the p tag with it on this.
    • When wrapping up text content, use meaningful elements like a p tag, change your span into using p tag.

    Aside from those, great job again on this one^^

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, really nice work on this one. I think the layout for desktop looks nice, the site is responsive as well when scaling the browser. For mobile state, there are breakpoints where you already show the one-column layout for the mobile view, yet there are still section where the contents are still using the desktop version. For example, at 700px, for the features section, the tab-controls are in a single column-layout but the content underneath is still using the desktop version. It would be nice to just show both the mobile version for each content. Also, at the 700px, the navbar's login wraps on another row.

    Here are some other suggestions for the site:

    • Maybe adding a max-width on the body tag or in a wrapper for the content. If you try to zoom out on your screen, you will see that the site's content are just stretching along. Adding the max-width will prevent this one.
    • Don't add outline: none on the * selector if you aren't going to provide a custom visual-indicator. The outline provides guide to the user when traversing focusable elements on the site. Try using the tab key to navigate the site, it is hard to know where you are at. Use the :focus-visible selector to add the custom outline or just remove your declaration.
    • For this one, the body doesn't need to use a flexbox since the content of the site are just sitting on a single-column. Also, your html elements that are nesting the site's content right now are just being direct child of the body which should not be since they should be nested inside their own landmark element.
    • On this layout, it would be really great if your navbar is inside the header, the main-content inside the main tag and the bottom-most part inside the footer tag. Your body tag should only contain these 3 landmark element:
    <header />
<main />
<footer />
    • Currently, you are using 2 nav to create the mobile and desktop version of the navbar which shouldn't be the the case. You should only use a single nav and just use css to make it adapt for mobile and desktop state. This way, your markup won't be using repetitive element.
    • Also, even I suggested using a single nav, right now, the mobile-state nav is not being hidden properly. When you are making something hidden, you don't just use the width or opacity as they are only hiding the content visually but they are still in the dom. You should always use visibility or display to show/hide the item.
    • For this one, the topmost nav element could use an aria-label="primary" attribute on it. The reason for this is that, I would later suggest nav on the footer tag. You should add the aria-label for a nav element if you are using the nav more than once on the page. This will make it unique.
    • The website-logo could be removed from the nav since it is not being treated as a site-link.
    • When you are using the alt for the site-logo, always make sure to use the website's name and not just logo because if a user traverses the images on the site, what does logo text will provide really. Also, avoid those words that relates to graphic when using value for the alt attribute.
    • Those navlinks could be wrapped inside a ul tag since those "list" of links.
    • When you are making a text uppercased, you don't write them directly uppercased on the markup because instead of screen-reader reading the word as it is, it will read the word letter-by-letter. Type them in lowercase and use text-transform: uppercase in css.
    • login is better using a tag rather than button on this one.
    • Don't nest a tag inside the button and vice versa as they just create 2 extra traversing. Use only a single one.
    • Don't use header inside the main as it is as it will be treated as a regular element. It would be better if it was replaced by div.
    • You don't need to use br tag to make the word wrapped in another row like what you used in the h1 tag. Use max-width for the h1 so that it will make the word wrapped if it needs.
    • The decorative images on the site like the hero-image could use an extra aria-hidden="true" attribute so that it will be totally hidden for screen-reader users.
    • If you are going to use article for the features use a single article to wrap both the tab-controls and the content below since they are related to one another.
    • For the features section as well, your controls are working fine but using button alone without any extra attributes addition is not informative at all. For this one, you can implement it as tabbed interface. You can have a look at my implementation on my solution for this. This way, it will be more accessible for the users.
    • more info should be using a tag since it is more likely to be link.
    • Also, I just noticed that almost paragraphs on the site are using br tag to make each text wrapped. Again, max-width will be much better.
    • Almost every content on your solution is using section tag but used incorrect. section tag as landmark is not really informative as it does not give extra information about the tag unless you are using aria-labelledby attribute on it. This way, it will read out the heading tag to which it points to. If the section ( not section tag ) of the site doesn't have a visible heading text, a div would be fine.
    • For your faq section, you don't use a tag to wrap the whole accordions. Use only a tag for contents that will navigate a user in a different page of the site of just navigate them elsewhere. A div replacement will be much better on this.
    • Right now, the accordion answer is still in the dom even if they are not toggled since you are only use max-height. Remember to use the above method I mentioned. If you don't want the trouble on this, use details tag instead. It is already accessible and is suited for this kind of structure.
    • For the cta section, your input right now currently lacks associated label to it or an aria-label to which will define the purpose of the input element. Always include it so that user will know what they need to give on each input. Make sure that label is pointing to the id of the input as well.
    • Right now, when a user submits a wrong form values, the error is only seen visually but not really linked to the input properly. A proper way of validating it would look like this:
    if ( input is wrong )
  input.setAttribute("aria-invalid", "true");
  input.setAttribute("aria-describedBy", id of the error-message);
else 
  input.removeAttribute("aria-invalid");
  input.removeAttribute("aria-describedBy");

    The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy.

    • The error-message as well is better to not disappear and just make it stay there so that user will still have a guide.
    • Another great idea to implement is to have an aria-live element that will either announce if the form submission is a success or not. This way, the user will be informed right-away on what is the status of the submission.
    • The button should have type="submit". Remember that when a button is placed inside a form element, it defaults to type="submit". So imagine if you have a close-button inside the form without specifying type="button" clicking the close-button will submit the form. Be aware of this kind of scenarios.

    FOOTER

    • Same with the company logo, use a more proper alt value.
    • Those 3 links could be wrapped again inside in another nav element since those are still your site's navigational links. The nav would have an aria-label="footer" to it so that it will be unique.
    • The footer-navlinks could be wrapped again inside ul since they are "list" of links. They are just the same structure as the header's navbar.
    • Those social-media links could be inside a ul element since those are "list" of links.
    • Each a tag that wraps the social-media icon should have either aria-label attribute or sr-only text inside it, defining where the link would take them. For example, you should use facebook as the value if the link would take the user to facebook.
    • Social-media image should be hidden since it is only a decorative image so use alt="" and aria-hidden="true".

    MOBILE

    • Hamburger menu should be using a button since it is an interactive component. When creating interactive component, make sure that you are using interactive element so that it will be accessible for other users.

    SUPPOSING BUTTON IS USED

    • The hamburger button should have a default attribute of aria-expanded="false" and it will be set to true when the users toggles it and vice-versa.
    • The hamburger button should have either aria-label attribute or sr-only text inside it which defines what the button does. You could use aria-label="navigation dropdown menu"
    • The img inside the hamburger-menu should have been hidden since it is only a decorative image.
    • Lastly, your placement of the dropdown and the hamburger is incorrect. The hamburger should be placed before the dropdown on the markup so that after the user toggles it, when they use the tab key, the focus will be set on the dropdown.

    Now, it is fine to have those lots of issues. Just always be aware in this kind of scenario and always try to aim accessible site. You can always take look at other people's solution or just search on the net for proper markup on a specific layout.

    Aside from those, great job again on this one!

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one and congrats as well for your first challenge here at FEM! Hope you had fun doing this challenge and learned from it:>. For desktop layout, it just needed it to be centered and it's good to go.

    Here are some other suggestions for the site:

    • It would be great to have a base styling of this:
    html {
  box-sizing: border-box;
  font-size: 100%;
}

body {
 margin: 0;
}

*,
*::before,
*::after {
  box-sizing: inherit
}

    This way, handling an element specially its size will be easier because of the box-sizing

    • In a webpage, your main-content should be wrapped inside the main tag so that it will be easy for the user to know and navigate on the main landmark. For this one, create a main tag that will wrap the 3 card.
    • Normally, I would suggest using a flexbox to center the layout on this challenge, but since there is not container that wraps the 3 card, using flexbox on the body will just ruin it. But if you manage to create a main, then use:
    align-items: center;
display: flex;
flex-direction: column;
justify-content: center;
min-height: 100vh;

    Styling on the body tag.

    • On each of the div card, instead of height use min-height. Using height will give an explicit /fixed sizing on the element making it not scale base on it's content, while min-height will allow the element to resize if needed.
    • Also, you could have create a single selector like .card that you can give on each of the card on the layout, this way you won't have to manually add the stylings on each of the sedan, suv, luxury selector.
    • For each of the car icons, they are only acting as decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
    • In any webpage, you should have a single h1 tag, usually it is the first heading tag inside the main tag. It describes the content of the site, but since there is no visible heading text on this one, you should create the h1 as screen-reader only h1. Have a look at this solution of Grace on the same challenge. Inspect the website and see how she used the h1 and the stylings applied to it.
    • Those "learn more" are better using a tag rather than button because on a real site, those would be page links for the user to visit and "learn more" about a specific car.

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop layout looks fine, just a little bit shorter. For responsiveness, if you go at 900px upwards, you will notice that the site hides the content and creates horizontal scrollbar. For mobile state, it looks fine but the top part is being hidden by the screen's ceiling.

    Here are some suggestions for the site:

    • Avoid using height: 100vh on a large container like the .wrapper container as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
    • For the image, you can use a more descriptive alt on it if you find the image meaningful. Right now, the text office is too broad on what is image's content is all about.
    • For the text-content of the site, the text-align should be set to left because right now, texts are centered.
    • For the .card__details, if you look at the content, those could be "list" of information about the company website, therefore you can use ul` tag on it.
    • Since I suggested ul, those span would be li and also, using just a plan span to wrap a content is not that great, you should always put content inside of a meaningful element.
    • Lastly, just addressing the responsiveness issue if you go around at 900 px upwards^^

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop layout looks fine, it's just a bit taller and some white-spaces could be reduced as well. The site is responsive and the mobile state looks great.

    Here are some suggestions for the site:

    • Adding a max-width on the body or in a container that holds content. If you try to zoom out on your screen, the content stretches along with the resize. Adding that max-width will prevent that and makes sure that the content will be consistent for the user.
    • Website-logo-link a tag should have either aria-label attribute or sr-only text inside, that describes where the link would take the user. Usually, website-logo directs user to homepage so use homepage as the value like `aria-label="homepage".
    • Remember that a website-logo is one of the meaningful images on a site so use proper alt for it. Use the website's name as the value like alt="clipboard".
    • Also for the site-logo, if you use tab through it, you will notice that it creates extra outline for the img since it is using margin-bottom which pushes it from the container. Use that styling on the a tag instead.
    • On general, using section is not bad but you can just replaced it with just div if there is already an existing visible heading tag on that section ( not section tag ) since if you traverse the site using screen-reader with landmark, when the screen-reader traverses the section tag, it doesn't read out a label text that signifies it as a landmark, unless you are using like an aria-labelledby if you want the element given an importance to.
    • Change those repetitive h1 into just other heading tag. Using h1 inside a section, article is not bad as well but hey, we just want to stick out with the semantic markup :>
    • For the images, you can add the clipboard app on the description since each images has the clipboard logo inside it, for example, alt="clipboard app running on mac desktop".
    • Those 3 icons on the workflow section are all just decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
    • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
    • Also, When using img tag, you don't need to add words that relates to "graphic" such as "logo" and others, since img is already an image so no need to describe it as one.
    • Use only the company's name as the alt and remove the word logo.

    FOOTER

    • Same for the site-logo, use the suggested method above about the link and the alt value.
    • Those 5 links should all be nested inside a single ul tag since those are related links. The ul could be wrapped inside by a nav tag since those are still your navigational links.
    • Those social-media links could be inside a ul element since those are "list" of links.
    • Each a tag that wraps the social-media icon should have either aria-label attribute or sr-only text inside it, defining where the link would take them. For example, you should use facebook as the value if the link would take the user to facebook.
    • Social-media image should be hidden since it is only a decorative image so use alt="" and aria-hidden="true".

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop layout looks fine for me, just that the hero-section is bit to taller, the text-alignment is using center instead of left and the white-spaces on the testimonial section could be reduce. For responsiveness, if you go at point 770px, you will notice that the site's content is being by the screen causing a horizontal scrollbar and the text of the site starts to get squished, the images doesn't respond and alignment is still not going well.

    David Turner already gave a feedback on this one, just going to add some suggestions as well:

    • It would be great to have this markup:
    <header />
<main />
<footer />

    This way, all content of your page will be inside their own respective landmark element. Remember that for the primary header, nest only the topmost part of the site which are the site-logo, navlinks and leave the hero-section inside the main tag because you want the header to be reusable for other pages, so only include what is needed on that.

    • The site-logo could be remove from the nav since it is not being treated as a link. If you insist on doing so, it would be nice to wrap the img by a tag and make sure to properly use an aria-label or sr-only text that would describe where the link would take the user.
    • Also, on the header tag's nav, you can add aria-label="primary" so that it will be unique since a another nav could be used inside the footer tag.
    • Those 4 links in the header could be wrap inside a ul tag since those are "list" of links.
    • Also, I saw a usage of height: 100vw on the .h-screen selector. Avoid using height: 100vh as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
    • Also, I would prefer to use rem value to give the height on the hero-section instead of vh unit so that it will stay consistent for all user. Try to inspect the layout in dev tools at the bottom, the hero-section changes a lot when you resize the screen's height.
    • The arrow-image on the hero-section is only a decorative image. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
    • For this one, just use a single h1 since you don't want it to be repetitive on the page. Use a single h1 only and change those into other heading tags.
    • It would be nice to use text-align: left as well on each of the section after the hero so that it will look like the design.
    • Those learn-more are better using a tag rather than button. On a real site, those would be page links and not like a control.
    • For this challenge, I would only use alt attributes on the site-logo and each of the person's on the testimonial section and the rest would be hidden using the method above since they are all decorative images.
    • For the cherry and the orange section, you can instead use those images as background-image for each container so that they will be easier to handle.
    • On the testimonial section, it would be nice to adjust the white-spaces on that part because right now, they are too much:>
    • For each testimonial, you can use this markup below so that it will be easier for the user to traverse the information using blockquote on a screen-reader:
    <figure>
  <img src="" alt={person name}>
  <blockquote>
    <p> {qoute in here}</p>
  </blockquote>
  <figcaption>
    person name
    <p>
      person role
    </p>
  </figcaption>
</figure>

    FOOTER

    • The background-color could be lighter so that it will match the footer's design.
    • Those 3 links after the site-logo could be wrapped inside a ul because again, they are "list" of links. Also, the ul on this could be wrapper inside a nav since they are still your site's navigation links. The nav should have aria-label="footer" so that it will be unique.
    • Those social-media links could be inside a ul element since those are "list" of links.
    • Each a tag that wraps the social-media icon should have either aria-label attribute or sr-only text inside it, defining where the link would take them. For example, you should use facebook as the value if the link would take the user to facebook.
    • Social-media image should be hidden since it is only a decorative image so use alt="" and aria-hidden="true".

    MOBILE

    • I noticed on the markup, you duplicated your navlinks for the mobile state which you don't really need. You just need to style the original navlinks to adapt for mobile state and desktop state. This way, you are reducing repetitive element on your markup.
    • Hamburger menu should be using a button since it is an interactive component. When creating interactive component, make sure that you are using interactive element so that it will be accessible for other users.

    SUPPOSING BUTTON IS USED

    • The hamburger button should have a default attribute of aria-expanded="false" and it will be set to true when the users toggles it and vice-versa.

    • The hamburger button should have either aria-label attribute or sr-only text inside it which defines what the button does. You could use aria-label="navigation dropdown menu"

    • The svg inside the hamburger-menu should have been hidden since it is only a decorative image so use aria-hidden="true" on it.

    • Also, the placement of your hamburger and the dropdown is incorrect. The dropdown should be placed after the hamburger so that after the user toggles it, the next focus is on the dropdown.

    • Lastly, in general, adjusting the styling on the site and addressing the responsiveness issue:>

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, great work on this one. The desktop layout looks fine, though the h1 is small, the text below it is not using a left-alignment and maybe adding more padding to the body or in a container so that the content won't look like spaced out. The site responds but if you go to at 500px upwards, you will see that the content is being hidden by the screen and creates horizontal scrollbar.

    Here are some other suggestions for the site:

    • Avoid using height: 100% or height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
    • The font-weight: 700 on the h1 could be remove since by default it already uses this.
    • For the .reviews selector, you don't really need to use grid on that one since the it's direct child is already a block element that will wrap on its own row. You just need to use margin on those item to place them properly like on the design.
    • For each .review, using article on them is not the best choice since an article tag usually contains content that is independent and could be redistributed on another page since it can sit on its own. For this one, you could just use div on each.
    • Each of the star icons, you can just use the image path as the value for the background property of each .review selector. Remember that background can contain multiple image, this way you won't have to create multiple img tags.
    • Though, if you insist on using img tags for the star-icons, then make sure to add alt="" and aria-hidden="true" so that screen-reader will skipped that image since it is only a decorative image.
    • For each person's img tag, it would make sense to use their full name as the alt value since it is already present.
    • Also, for each testimonial, you could use this markup below so that it will be easy for screen-reader to traverse it using blockquote:
    <figure>
 <img src="" alt={person name}>
 <blockquote>
   <p> {qoute in here}</p>
 </blockquote>
 <figcaption>
   person name
   <p>
     person role
   </p>
 </figcaption>
</figure>

    Though you just need to use grid on the figure to place each item properly since a figcaption should be the first or last item of a figure element.

    • The verified-buyer should not be using a heading tag since it doesn't really give information on what a section/part of the layout would contain. A p tag on it would be nice.
    • Lastly, addressing the responsiveness issue on the site:>

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome for another challenge. The desktop layout looks fine, though the hero-image is quite big/long and making the site a bit longer because of some white-spaces as well. The site is responsive but if you go at 340px above, you will notice that the site is hiding the content of the page and creates horizontal scrollbar.

    MordenWebDev already gave a feedback on this one, just going to add some suggestions as well:

    • Avoid using height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
    • Again, a page needs to have a main tag to wrap the main-content so that user will be able to quickly navigate the content using landmark elements. Use main on the .container.
    • Remember that a website-logo is one of the meaningful images on a site so use proper alt for it. Use the website's name as the value like alt="ping".
    • Your input right now currently lacks associated label to it or an aria-label to which will define the purpose of the input element. Always include it so that user will know what they need to give on each input. Make sure that label is pointing to the id of the input as well.
    • The notify-button should be using a type="submit" attribute. Remember that when a button is placed inside a form element, it defaults to type="submit". So imagine if you have a close-button inside the form without specifying type="button" clicking the close-button will submit the form. Be aware of this kind of scenarios.
    • Right now, the error message or the error in general is only seen visually but not really linked to the input properly. A proper way of adding errors would look something like this pseudocode:
    if ( input is wrong )
  input.setAttribute("aria-invalid", "true");
  input.setAttribute("aria-describedBy", id of the error-message);
else 
  input.removeAttribute("aria-invalid");
  input.removeAttribute("aria-describedBy");

    The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy. If you like, have a look at this simple accessible form snippet that I have. Let me know if you have any queries about this one.

    • For the hero-image, you could use a more descriptive alt, it could be something like `ping's application user dashboard" or if you could come up with a more descriptive.
    • Those social-media links could be inside a ul element since those are "list" of links.
    • Each a tag that wraps the social-media icon should have either aria-label attribute or sr-only text inside it, defining where the link would take them. For example, you should use facebook as the value if the link would take the user to facebook.
    • Also, I would put the social-medias inside the footer since those aren't really part of the main-content of the page.
    • Lastly, just reducing some sizes perhaps of the image and some white-spaces:>

    Aside from those, great job again on this one.

  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop layout looks great, just the overall layout is big bigger compared on the design. The site is responsive and the mobile state looks fine, though it could use smaller font-size for the mobile state since the heading text is quite big right now and some components as well.

    For some other suggestions, here are some:

    • Heading tags doesn't need to use font-weight: 700 since by default it is already bold ( 700 ).
    • Since it has a form-component, it would be nice to use form tag for the .input selector instead of div so that it will be clear on the markup.
    • Your input right now currently lacks associated label to it or an aria-label to which will define the purpose of the input element. Always include it so that user will know what they need to give on each input. Make sure that label is pointing to the id of the input as well.
    • When submitting an invalid form, I suggest making the error-messages stays present until the user submits again the form, because right now it is hiding the error-messages after a couple of seconds and a user might not be fast enough to read those error-messages.
    • Also, those error-messages are only seen visually but not really linked to their respective input tag. A proper way of adding those errors would look something like this:
    if ( input is wrong )
  input.setAttribute("aria-invalid", "true");
  input.setAttribute("aria-describedBy", id of the error-message);
else 
  input.removeAttribute("aria-invalid");
  input.removeAttribute("aria-describedBy");

    The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy.

    • Those error-icons are only decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag.
    • Another idea to implement is to have an aria-live element that will announce if the form submission is valid or not. This way, the user will be informed right away on what is the status of their submission.
    • If you like, you can have a look at this simple accessible form snippet that I have. Let me know if you have any queries about this one.
    • Those terms and services should be acting as link since on a real site, they are interactive so that user can see those information on another page. Use an a tag for that text.
    • Lastly, just about the layout for mobile state, reducing some font-size and just making the layout a bit smaller:>

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, great work again on this another solution. The desktop view looks fine, it's just a bit pushed down, the h1 is bit bigger and the box-shadow for the form could be made more smoother. The site is responsive though and the mobile state looks fine as well.

    For some other suggestions, here are some:

    • On this one, you could just use div instead of header and put it inside the main tag since this is just a single main-content. Typically, a primary header consist of the site-logo, navlinks and maybe some controls.
    • Also, since this is just a one-column layout, you could just omit the display: grid on the body tag and just add like a min-height on the form or just add sizing on the elements if needed. Because I noticed that when you resize the screen's height when in dev tools at the bottom, the layout shifts a lot and reduces it's height.
    • For the slider, the text...page-views is not really suited to a label on that one since it is quite ambiguous on what does it mean and if you look at it, changing the slider changes the pricing, it would be nice to make those 2 announced. For this one, you could create an aria-live element that will announce the changes for the 2 items or use an output tag on those 2 items, the page-views and the pricing. You could either one.
    • For the input type='range', it would be nice to use either aria-label attribute or a sr-only label tag. Then use a meaningful text to be used as the value, it could be like pricing selection something like that or if you could make up more meaningful, that would be nice.
    • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
    • For the billing section, since the two are selection of billing choice, instead of using checkbox, you should use radio-buttons whenever there are selections where you can only select one. For that one, you should use a fieldset tag to wrap the radio-buttons, a sr-only legend tag to which contains a text that will describe the set of radio buttons, have 2 label it will be the month and yearly text. I don't recall if I use this markup before when I did this, but I have this simple snippet about theme-toggle if you like. Though it is different, the markup is the same on both. Let me know if you have queries about this one.
    • Also, this layout is basically a form where the start my trial text is the submit-button, therefore it would be nice to use form tag inside the main to wrap the component and use button type="submit" instead of a tag.

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop view looks great, the site is responsive but at mobile state, at around 340px upwards, the site's contents are being hidden by the screen because the 1000gb text part is not responding well to the site's changes.

    Byron already gave a helpful feedback on this one, just going to add some suggestion as well:

    • Since a main tag is needed for every page to distinguish the main-content of the site, replace the section to using main on the .container selector.
    • Remove the height: 100vh on the .container. Avoid using height: 100vh on a large container this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.
    • Instead of using the max-width on the .container use it on the body tag, this way, even if you zoom out on your screen, the layout will stay consistent or maybe you can just add background-size: 100% on the .container.
    • Remember that a website-logo is one of the meaningful images on a site so use proper alt for it. Use the website's name as the value like alt="fylo".
    • Also, when using img tag, you don't need to add words that relates to "graphic" such as "logo, image" and others, since img is already an image so no need to describe it as one.
    • Those 3 icons below the site-logo are just decorative img tags. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
    • Also, it would be nice to change the span that holds the each of the icon to using a button since it looks like the component is interactive or use div.
    • On a site, always have a single h1. Since there are no visible text that are suitable to be h1, the h1 would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, the h1 would have like sr-only class and the text-content should describe what is the main-content is all about. The h1 could be placed as the first text inside the main.Have a look at this simple snippet of mine using sr-only h1 comments are already included in there, but if you have any queries just let me know.
    • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span, strong to wrap the text.

    MOBILE STATE

    • Don't use width: 100vw since this will only add a horizontal scrollbar at the bottom, since this value does not account the vertical scrollbar's width.
    • Adjust the .progress-bar so that the site will properly resize and won't create the horizontal scrollbar since the content will be prevent to overflow.

    Aside from those, great job again on this one.

    Marked as helpful
  • Raymart Pamplona•16,040
    @pikapikamart

    Hey, awesome work on this one. The desktop view looks fine I think, just needed those padding or gap between each card so that it will set like a boundary. Also, if you could adjust the box-shadow a little more to dark side, that would be really nice. The mobile state looks fine as well, just for the breakpoint, it is too early. Right now, you are using 0px - 1250px to show the mobile state which is really big for only the mobile state to occupy since the desktop view could use more of those screen time. Adjusting that would be nice.

    For some other suggestions, here are some:

    • For this, you should only have a main tag as the direct child of the body. Put the 2 heading tag inside the main since it is part of the main-content of the site.
    • For the h1, the br is not needed on that one. What you can do is that, add a max-width on the h1 so that it will limit the size and that will wrap the second text on another row. Just remember to add margin: 0 auto to center it.
    • The text after the h1 is not really a heading tag. Usually, a heading tag is not like a long sentence and it should describe or give information on what the section/part of the layout would contain. Use p tag on this one.
    • main tag doesn't need top use role="main" since it is already a main.
    • Also in general, adding a max-width on the body for example will make the site more consistent. If you zoom out on your screen, you will notice that the layout's content stretches along. max-width will prevent this one.
    • On each card, you could just replace the article by div tag since it just looks like a regular content of the page to which can't be reused on other pages where article content can be. A div would be fine and each of the heading tag will be enough to structure it.
    • Also, since you are removing the h2 after the h1, use h2 on the h3 on each card so that you won't skip a heading level.
    • For all the icons on the cards, hide them since they are only decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
    • Lastly, just keep in mind about the first suggestions about the padding and box-shadow ^^

    Aside from those, great job again on this one.

    Marked as helpful