React, Styled Components, React Router, React Helmet, & FormSpree API

Community feedback

• 

  htmlHxcker•70

  @htmlHxckerPosted about 3 years ago

  Hi Fola, really nice work here, In fact, I had to look very closely to find errors :), anyways here are the things I noticed:

  1. For the file structure: You have a global folder in the Components folder which houses Global components but then there's another folder that houses Global styles, don't you think it'll be better if the global style was in the same folder as the global components?
  2. Some pages take a lot of time to load their Images as much as 4 seconds which end up causing content shifts when the image loads, I think this is because you wait for data to be loaded, do you think you could load all the images once the page loads for the first time before the user starts navigating to other pages as this happens when I go to other pages e,g the about page, Perhaps you could use a preloader too.
  3. On the contact page, If I input a phone number in the international format i.e +1 (408) 735-7638 I get the error: "Phone should be a number!" This is kinda weird, upon inspecting I found that: I. You used a type attribute set to text instead of tel and, ii. You're using isNan to validate the numbers, I think you could fix this one by using RegExp as you did for email. Here's a function to that effect:

    let formatted = (' ' + phoneNumberString).replace(/\D/g, ' ');
    let match = formattedmatch(/^(1|)?(\d{3})(\d{3})(\d{4})$/);
    if (match) {
      let intlCode = (match[1] ? '+1 ' : '');
      return [intlCode, ' (', match[2], ') ', match[3], '-', match[4]].join(' ');
    }
    return null;
  }
  

  Gotten from https://stackoverflow.com/questions/8358084/regular-expression-to-reformat-a-us-phone-number-in-javascript

  That's as many faults as I could find, I have to say the project is very polished and the code is very easy to read so props for that.

  Marked as helpful

  2

  ---

  

  Folarin Akinloye•1,240

  @folathecoderPosted about 3 years ago

  @Hakymreality Thank you very much! I will make corrections ASAP. I appreciate the feedback! 😎

  1
• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout is really good in desktop, it is very responsive and the mobile layout looks really great as well.

  I don't usually review react codes since everyone has their own way implementing, but if I do, I just usually check if things are rendered when they are needed, because some including me as well sometimes forget to prevent re-renderings, maybe state are not used properly, memo is not used, context is used for large data, those kinds. But for this one, I would just go with markup or accessibility.

  Suggestions would be that:

  • max-width: 100% and min-height: 100vh on the body is not needed since by default body takes full width and since there is a large content, you won't need min-height: 100vh.
  • Navlinks could use a ul element since those are "list" of links, it will also inform a user on how many items or links are there in the list.
  • You don't need to add extra aria-label on the navlinks, the text inside them is enough to inform a user to where the link would take them.
  • I don't really know about the primary="true" attribute set on the hero-section's a tag. What is that?
  • Also, if the aria-label's value is too long or just any aria-label it would be great to instead use an sr-only element inside them, because aria-label text are not translatable for other languages that is why sr-only is much preferred since text-contents are translatable.
  • I don't know if I would put alt on the hero-section's image. Hmm, now i'm confused haha.
  • You don't need as well aria-labels on the 3 links after the hero-section since the text inside it has the text where the link would take the user.
  • The 3 images used before the cta could be hidden alt="" and aria-hidden="true" since they are just vector images and are just decorations.

  FOOTER

  • To be honest, using homepage as the aria-label is enough to describe where the link would take the user, same for header's website-logo-link.
  • Navlinks should be using nav since those are still your navigational links on your page, nav is not only limited to the uppermost part on the header but on the footer as well. Use ul tag as well on the links since they are "list" of links.
  • Also, when navigating on a different page of the site, it would be great to add an aria-current="page" on the current link where the user is at.
  • On the social media links, ul could be great since again, "list" of links.
  • Each img on the social media should be hidden since they are just icons/decorations so use alt="" and aria-hidden="true" on them.

  MOBILE

  • Dropdown button should have aria-expanded="false" as a default attribute, it will be changed to true if the user toggles the button.
  • Your dropdown is only hidden visually, right now you are transitioning only the opacity to make it seem like invisible, but using only this is not enough. It is still visible for assistive tech and you don't want users to see a supposed hidden items. Instead, add visibility: hidden on the default state, and use visibility: visible on the toggled state, this way it would totally hidden for users.

  I can only review right now the homepage since I am past my bedtime hours :>

  But still, really great work for this one. I only got to see few on these guru challenges so really awesome for you to pull this one off.

  Marked as helpful

  1

  ---

  

  Folarin Akinloye•1,240

  @folathecoderPosted about 3 years ago

  @pikamart Wow! Thank you very much for the feedback. I really appreciate the time and effort you took!

  0