Styled Components, React, hosted on Heroku

Community feedback

• 

  T

  Alex•2,010

  @AlexKMarshallPosted almost 3 years ago

  Careful about accessibility. Make sure you always test your designs with a keyboard to make sure they work.

  If you remove the built in :focus-visible styles, you must replace them with something equally high-contrast. On the search input you've removed the styling and so there's nothing to indicate that I'm focused on the field.

  Make sure to use proper alternative text for images, or an empty string if they are decorative or convey no extra information. An alt text of "avatar" doesn't mean anything to someone viewing the site. If the Github API provides alt text for the user's avatar, then that should be used. If not then it should be an empty string.

  Taking a look at your code, you're using a lot of very nested selectors in your CSS. That's going to cause problems with maintainability due to massively increased specificity. Prefer to use single class names and target those. What that means in styled-components is that you'll create individual styled components for the various elements. Rather than nest them all under a single styled component. That way it becomes far easier to override styles when you need to.

  Marked as helpful

  1

  ---

  

  P

  Ahmed Abdelaal•220

  @shrki416Posted almost 3 years ago

  @AlexKMarshall Excellent suggestions, thanks so much for taking the time to view my solution and offer feedback, very much appreciated :)

  0
• 

  Agata Liberska•3,995

  @AgataLiberskaPosted almost 3 years ago

  Hi @shrki416!

  Nice work, your solution looks great :) I only have a few suggestions:

  • On smaller end of mobile viewports, the app overflows the screen as you set a constant width. 320px is not an unpopular viewport size so it would be good to cater to it :)
  • Not sure what the brief was, but I think it would be better for name to stay blank if there is no name set, rather than defaulting to Octocat. You could provide initial state for Octocat info to display when app first load and then override that?
  • I'd love for the Twitter thing to be a link to the twitter profile if it's available and maybe don't have a link for the website if one isn't provided (have the text 'not available' rather than anchor tag)
  • Also, I'd love to be able to switch themes with keyboard to make it accessible - you could look into using a button or a pair of radio inputs there?

  Hope this helps, and again, really nice work!

  Marked as helpful

  1

  ---

  

  P

  Ahmed Abdelaal•220

  @shrki416Posted almost 3 years ago

  @AgataLiberska wow! Excellent feedback. thanks so much for the suggestions above :) I'll go through and make those changes. Thanks again!

  0