Design comparison
Solution retrospective
Hello !
Thanks for taking some time to review my solution to this challenge. This is my second submission overall.
With this challenge I learned more about:
- Creating a layout with CSS grid.
- CSS custom properties.
- SVGR.
- End to end testing with Cypress.
All feedback is appreciated. However, if I had to ask for something in specific, I would ask your opinion about:
- My CSS grid implementation.
- Accessibility. I'm a beginner in regards to this topic, so any feedback on this would be great !
Community feedback
- @pikapikamartPosted about 3 years ago
Hey, awesome work on this one. Layout is great in desktop and I suggest first to default search the octocat like in the design so that it won't feel empty at first, it is responsive just a little issue on around 580px going to 525px horizontal scrollbar appears. Mobile layout looks really great.
Some other suggestions would be:
- Don't style the
html
leave those on thebody
tag. - The theme toggle works but it would be better to use
input type="radio"
on those because usingbutton
is not accessible enough in this kind of component. - The reason that you should use radio button is that, color theme is a selection right and radio buttons are intended for those, either select light or dark. You will need 2 radio buttons each having their own
label
, they will nested inside afieldset
along with alegend
attribute, thelegend
could be a screen-reader only. I don't have this challenge, but you can take a look at my todo-app-solution inspect the layout and see the structuring of my color-theme. This is much accessible. - Theme
svg
should be hidden so usearia-hidden="true"
onsvg
that are purely decorative. - Your
input
'slabel
should have a descriptive text-content that defines what is the purpose of theinput
. Thelabel
would be a screen-reader only text, usesr-only
class, the text-content could beEnter githhub username
, that magnifying glass should just be abackground-image
of theinput
. - The
form
works but since these is no indication on whether it has searched someone or not for "not" sighted users. I suggest to add anaria-live
element which will announce if there is a user found or if there is not. This way, it will be more accessible since the error-message is not being used properly as thearia-describedBy
of theinput
element. - Again for the
input
if there is no user found, maybe add anaria-invalid="true"
on theinput
this way users will be informed that their input is wrong. Also, add anid
attribute on the error-message, thisid
will be referenced by what I said,aria-describedBy
on theinput
as well, this way they will know what kind of error has it produced. You will be using.setAttribute
and.removeAttribute
if you are going to useref
in react or just create a state to check for theinput
attribute. - The
alt
for the user's image should be their username. Avoid using words that relates to "graphic" such as "avatar" in thealt
attr. - Don't just use
span
to wrap text-content, usep
tag on them. - You could use
table
element for the 3 section after the person's name or useul
element since it is a "list" of information. - On the 4 items on the bottom part, use
ul
on those since they are "list" as well. - Each icon should be hidden by
aria-hidden="true"
.
Aside from those , site looks really great. Great work again on this one.
Marked as helpful1@omonteonPosted about 3 years ago@pikamart Thank you for such a great feedback ! It's really cool that you took the time to comment on all the improvements related to accessibility. I'm sure I'll learn a lot implementing all your suggestions.
** EDIT **
- Added octocat as default.
- Fixed responsive issue between 525px and 580px.
- Changed global styles to be scoped to the body.
- Started working on using a fieldset and radio buttons for the theme switch.
- Added aria-hidden to decorative svgs.
0 - Don't style the
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord