Looking to hire developers?
@AlexKMarshall
@magdakok
Submitted
Hello :) The goal of this project was to practice React (with React Hook Form) and Typescript. Styling was not my main focus here, but I really enjoyed using CSS Modules. I'd love to hear your opinion - I'm sure there's a huge room for improvement when it comes to React + TS.
This is looking very good. Functionally it all works well. I love the whimsical but still very clear focus states, and the keyboard navigation on the custom radio group is all good
You've said styling wasn't your focus so I won't spend too long on it, but when I load the site (or hard refresh) I get some layout shift (the content seems to jump up twice before settling on its final location). My guess is this could be caused by the useMobile hook - useEffect runs after paint, so could cause jumps if any layout depends on it. You could try replacing it with a useLayoutEffect which runs before paint. Even better would be to avoid this hook completely and do things entirely in CSS, that'll remove any possibility of the wrong value being computed. I get a similar layout shift on the confirmation page too, though here I think the image not having enough reserved space may have been the issue.
In terms of the typescript this looks ok, there's just a few comments I'd make:
types file or folder, particularly for types that are only used in a single place, hides things too much in my opinion. If you need to declare a type, declare it inline, or directly above the function that needs it. If you need to use it somewhere else too, export it from that file. Reserve a global types folder/file for truly generic type utilities or something that really isn't related to anything specific. I would expect there to be few of these.For the React code there isn't anything wrong here as such. However, for my tastes the code has been abstracted too much. The result is some components that handle many different things, and as a result need to receive 10-20 props. This has a two-fold effect. First, I find it makes the code quite hard to reason about. It also makes the components very coupled. If you want to change something in this code (maybe add a new page to the form) that change will have to touch many many files. And will involve re-testing the existing pages.
It's hard to give a small piece of advice here, and as this works you may want to ignore this anyway. But, I'll try and put together a few thoughts:
useCurrentStep hook - this is very coupled to the app component. It can live in the same file, that way there's less jumping around the code-base when trying to figure out what a particular component does. You may find you don't even need it to be a custom hook at all.UniqueSteps component is too complex for my tastes. It tries to handle every page of the form and the confirmation page. Instead, I would prefer to keep the step logic in app, and render a separate component for each step. Those components can then be responsible for their own instances of useForm and encapsulate all their form logic, only exposing an onSubmit and initialValues to app which can manage the global form state (but none of the validation)data.json. This could be a really neat way to have forms that are customisable by content editors from a CMS. However the current implementation doesn't really go that far. While all the data is in a json file, the UniqeSteps and other components still have to know about what's in that json file. In my opinion this has now split the places that I need to read to understand the logic, without providing benefit. Personally, I would delete the data file and manually write the jsx out in each form component. Long jsx is preferable to logic that is too spread-out.I hope some of this is helpful and not too vague. If you've got any questions or want me to go a bit deeper on any of this I'd be happy to.
Marked as helpful
NOTE: I intentionally deviated from the design in places to improve the accessibility.
I've added some To-dos to the Readme, including:
I'd particularly like suggestions on how to validate the password field against the individual regex patterns with React-Hook-Form (i.e. show only the failing criteria).
This is great. And for a first project with React, it all looks idiomatic to me. I certainly agree that form validation can be a bit of a rabbit hole. Using React Hook Form here was the sensible option.
For matching off against individual parts of the password validation, I think the way to do it is to break up the regex. Have one regex for each criterion, and use that inside a custom validator function. This is with the caveat that I have yet to try it myself.
Another alternative would be to use a schema library like Zod. That way you can define all your rules, and the failure messages associated with each, in the schema. Then pass the schema to RHF in one place rather than wiring up all the field validators separately. If you were building an app with a lot of forms, I would definitely recommend this. Though for a single form the extra bundle-size might not be worth it.
A couple of other things I noticed while digging through the code:
In general all the components are nicely composable - accepting children. In the page intro component though there's a mix of a heading text prop, plus children. My personal preference is to avoid those kinds of text props in React. Mainly because it becomes more difficult to pass markup into them. If, for instance, you wanted to wrap a specific word in the heading in a span, you now have to pass in a fragment which feels awkward. I would probably export a PageIntroHeading from the same file and use that to wrap up the h1 styles and let the consumer pass it in
<PageIntro>
<PageIntroHeading>Learn to code...</PageIntroHeading>
<p>See how other dev...</p>
</PageIntro>
In the form component, you've extracted the input styling into its own string to be reused in the various fields. I would try to avoid that. Unless you explicitly add config to target named variables, I believe the tailwind compiler will struggle to properly purge unused class names, as it will just look through the jsx for the className prop. That said, it looks ok in the deployed site, so Tailwind may have made the compiler more intelligent at spotting classes wherever they are used.
I personally would prefer to extract a styled <Input/> component that packages up the class names instead. Or you could use @apply and create a component class for it. At work we tend to do the former so there is only ever one way that a component can be styled, but both are valid options.
One more small thing. You might want to accept an additional as prop in the VisuallyHidden component. That way you can pass in either a div or a span, which would mean this can be used in places where one or the other of those would be invalid HTML. E.g. if you needed to wrap a <p>
function VisuallyHidden({as = 'span', children}) {
const Element = as
return <Element className="sr-only">{children}</Element>
}
In general, I try to avoid as props. They can make the components much more complicated. Especially if you try and do tricks like make your buttons work as links etc. But I think here is a good use case for them.
Marked as helpful
@nnall
Submitted
Looking for feedback on updated QR Component
Well done for submitting your first challenge. It looks good, there are just a few things that would be worth sorting out. It will be helpful to get into these good habits when first starting out, as they become much more important as projects get more complicated.
width: 100vw; height: 100vh;. The width is unnecessary here. The body will always take up the full width of the viewport unless you make it do something else. In addition, 100vw doesn't take into account things like scrollbars or device notches, so will often cause overflow. As it is both unnecessary and occasionally dangerous, just remove it.min-height: 100vh here in order to vertically center the card. But make sure it is min height, not just height. By using height you make content get cut off if the screen is shorter or the content longer than you planned. Min height avoids that problemwidth anywhere really. You have set width: 340px on your main element here. But what if the available space is less than that? You'll get overflow. Instead, use max-width which will allow things to shrink if necessary. In general, never use width or height other than for very small things like fixed-sized icons.box-sizing: border-box on all elements will make things more predictable. Here you've set it to inherit which isn't a particularly useful option.<main> element as it needs to push its content away from the sides rather than margin on the children. Margin can be used to create vertical space between the children, or if the space is equal you can use gap on the parent instead, it's simpler to maintain.margin: 4% - aside from generally avoiding margin, where you do need to use it, use a predictable unit like 1rem. Using percentages will make things much harder to reason about when many elements are interacting with each other.<h1>. Headings must go in order like a table of contents.display: block to avoid the line height issues caused by the default display: inline. This would generally form part of the CSS resetfont-size: 1.08rem seems an odd choice. 1rem would be fine (and is the default)width: 80% on things like the paragraph and the attribution. It's unnecessary, the spacing should be determined by the padding on the parent.Prettier in your code editor and make sure it is set to format on save. There are a lot of formatting inconsistencies in the code. When your code gets longer and more complicated this will make it far more difficult to read, so let an automated tool solve this for you.@paulauskas-linas
Submitted
It was a very refreshing challenge. Got stuck on setting event listener on body. It was a case of capturing and bubbling. My event was triggering bubbling and I needed to capture, so simple true statement saved the day.
If there is anything I can improve on cleaner, more efficient code, please leave a comment!
Hey there, great effort on this. The styling looks good, and it works fine with a mouse.
However, it doesn't work with a keyboard. There are just a few things needed to fix that. Good job on using radio buttons for the form. But, you've used display: none, which hides them from a keyboard and screen reader. Instead, use a technique to hide them visually, but keep them accessible https://piccalil.li/quick-tip/visually-hidden/ - and then, make sure to add a :focus-visible indicator (probably a bold ring around the label), so you can see where you are when using the keyboard.
For the form submission, make sure to use the submit event on the form, not the click event on the button. This will make the form work with the return key and a mouse. You can also use the FormData class to get the data from your form, rather than having to make a loop through the radio buttons.
const formData = new FormData(formElement)
const rating = formData.get('rate')
Marked as helpful
@joaquinrftech
Submitted
Is there another way to make the hover of the NFT image?
For the hover effect, you need to think about what that means. Generally, having something highlight on hover implies that when we click on the thing, something will happen. Typically that will either be navigating the user to a new page, or executing some kind of action (submitting a form, opening a modal dialog etc).
In this case you don't have to code that interaction itself, but you do need to have a clear idea in your head what you think it should do. In my mind, clicking on the image in this design would open up a bigger version in a modal. But navigating to a full detail page for the image would be an equally valid decision.
So now the important thing is what to do in the HTML to make this correct. If clicking on something navigates us to another page, that means we have to make it a link. So it needs to be wrapped in an anchor element <a href="/some-url"> . If clicking it causes some other action, that means it's a button, so it must be wrapped in <button>. If you don't do that, then the hover effect just becomes some meaningless decoration, which is not what we want on the web.
So then once you have the HTML right, for whatever you've decided is the appropriate behaviour, then you can do some styling. Whether you've chosen a button or a link, the CSS will be the same.
a.main-photo:hover {
/* some styles */
}
or
button.main-photo:hover {
/* some styles */
}
That will deal with the hovering. But, that's only for people that navigate with a mouse. Lots of people can't, or choose not to, so we need to handle keyboard navigation as well. For that we can use the :focus-visible pseudo class. Sometimes the hover effect and the focus effect should be different. Here I think it makes sense that it's the same, so your CSS would become
a.main-photo:hover, a.main-photo:focus-visible {
/* interaction styles */
}
@tomatas95
Submitted
Hey there, your site deployment has not worked. This is because you have a nested folder inside your github repository. Github pages is looking for an index.html file at the root of your project. If you move all of the files out of that nested folder into the top level, everything should work then.
@timmld
Submitted
Hey, it looks like your live site url isn't working. This is because you have a nested folder inside your repository, and your work is inside that. Move all the files from there to the top level of your repo, and everything should work fine then.
Most build tools including github pages are looking for an index.html file at the root of your project.
@Christ-Kevin
Submitted
I don't have a particular question on this project but I would be grateful for any suggestions that can make me improve my coding skills
This is looking very good, there's just a few things that would be worth taking a look at
The add to cart button:
The perfume ribbon text:
P E R F U M E directly in the html. A screen-reader will read that as individual letters, not a wordPerfume in the html, and then use css text-transform and letter-spacing properties to style it like the designThe prices section:
gap instead for more flexibilityflex-wrap: wrap so it can wrap onto a new line if there's not enough space for it to fit in one lineThe image:
<main>object-fit: fill, so at a couple of screen-sizes the image stretches out of its aspect-ratio. To avoid that use object-fit: coverIn general though these are mostly fairly minor points that are easily fixed.
Well done for making it very responsive, avoiding almost all overflow, writing good alt text for the image
Marked as helpful
@ivcp
Submitted
Learning React. Any feedback welcome
Well done on this. It's pretty close.
Good choice to use radio buttons for the input, but there's a small problem. When you use display: none on them, then they are not able to be focused, so you can't use this with a keyboard.
Instead, use some kind of visually hidden technique on them instead, so they're still accessible to screen readers and keyboards. Here's an example https://piccalil.li/quick-tip/visually-hidden/
Marked as helpful
@leoikeh99
Submitted
All feedbacks are welcome
From a visual perspective, this does look very good. The site responds very well to different screen sizes, and zoom.
However, there are some very serious accessibility problems that make this completely unusable for anyone using a keyboard or assistive technology like a screen-reader
onClick handlers to things that aren't a <button> - your quantity inputs, the +/- spinners and the cart button itself are all unusableprefers-reduced-motion - this is a serious problem for people who can be made dizzy or to feel sick from these kind of animations. Also for people who just want to switch them off because they find them annoyingalt text, so these must be real images in the dom, not just CSS backgrounds. They're not just for decorationMarked as helpful
@ranjanamukhia
Submitted
I have revisited this Challenge and i would really like to get feedbacks for it. Some of the points i can think of :- Some of the points which i think of are:- 1.why my screenshot is not similar to the design screenshot..even though on browser mobile and desktop it looks almost same to me ? 2.is my media query breakpoint correct? 3.Any suggestion related to good practice is also welcome.
This looks good to me. The one thing I'd change is to add some padding to the .card
That way you can remove the max-width from the text and the image, and a few of the margins.
In general, if you're looking to keep the children away from the sides of a parent, you want padding on the parent. It's the more common spacing property to use. Margin would come into play when adding gaps between siblings, like between the image and the heading, and the heading and the paragraph text. This can be achieved with a margin-top and margin-bottom on the heading
Marked as helpful
@tengxuanp
Submitted
The rating buttons required double click to hightlight in orange, because useState resets focus whenever value changes, not sure how to make it work on single click.
Hey there. The problem you have described is likely because you have defined your Button component inside the body of the Rating component. This is invalid React code, because the component definition will get re-created every time Rating re-renders, causing all sorts of unexpected behavior.
The quick fix is to move the definition of Button to the top level of the file.
However, for full accessibility, this should be a <form> with <input type="radio"> inside it. Doing it that way will allow the browser to properly communicate that this is a form, and it has something inside it that you can only select one of. You can also get rid of the useState because the form will manage its own uncontrolled state.
Marked as helpful
@toonchavez8
Submitted
Hey there, the styling of this looks very nice, and it looks good on a small screen. The animation is a nice touch.
Did you try testing it with a keyboard though? It doesn't work for anyone that isn't using a mouse, unfortunately.
To fix that you'll want to look into using <form> and <input type="radio"> elements. As a bonus, doing that will allow you to remove almost all the javascript, and rely on pure HTML and CSS for almost everything.
Marked as helpful
@thomashertog
Submitted
trying out challenges that require JS all comments on JS code (as well as accessibility) are mostly welcome as those are the areas i want to improve
Hey there. In terms of accessibility, when you build something interactive (like this is). You must test it with a keyboard. Ideally, test it with a screen reader too, but a keyboard will get you a lot of the way there.
If you try and use this with a keyboard you'll see it doesn't work. It's not possible to focus on the rating values and select one, you can only press the submit button.
You're not that far off. You're using the right element <input type="radio"/>. But because you've set it to display: none it becomes inaccessible. You'll need to use a method to hide it visually, but keep it accessible for screen readers and keyboards - here's one way you can do it https://www.a11yproject.com/posts/how-to-hide-content/
In terms of the javascript, the only thing I'd suggest is that you attach your event listener to the form's submit method, rather than the button's click. Other than that it looks nice and clean
Marked as helpful
@ToniHunter274
Submitted
Was finally able to host my website.. Kindly preview the code.. Any suggestions will be highly appreciated. Thank you @AlexKMarshall for helping me in hosting this site.
Your index file has an extra . in the filename. I suspect that may be breaking your deploy
Marked as helpful
@SOURABH358
Submitted
This is my first react App do tell me any improvements in my code. Your reviews help me a lot. ✌
Hey there, the styling of this looks very nice, and it looks good on a small screen.
Did you try testing it with a keyboard though? It doesn't work for anyone that isn't using a mouse, unfortunately.
The reason this doesn't work with a keyboard is that you've used non-interactive elements for things that a user is supposed to do things with. The rating buttons are just <div> elements here. Anything that a user should interact with needs to either be a link, a button, or form control. Those elements have accessibility built-in. Using a non-interactive element won't work, even if you put an onClick handler on it.
The easy fix for this is to make those ratings radio buttons instead. Something like:
<fieldset>
<label>
<input type="radio" name="rating-group" value="1"/>
<span>1</span>
</label>
<label>
<input type="radio" name="rating-group" value="2"/>
<span>2</span>
</label>
etc
</fieldset>
By doing that you'll get built-in keyboard support. And you won't have to do stuff in the javascript to style them as you can get which button has the :checked state directly in the CSS.
Marked as helpful
@kenneth0417
Submitted
Any feedback please on how can I improve in design.
Hey there, this looks good, and is nice and responsive on smaller screens.
However, it's not a very accessible implementation. Try to use this with a keyboard and you'll see what I mean.
The reason this doesn't work with a keyboard is that you've used non-interactive elements for things that a user is supposed to do things with. The rating buttons are just <li> elements here. This means you have to add extra javascript to handle interactions, and it's very easy to miss all the things needed to make this work with keyboards, screen-readers etc.
The easy fix for this is to make those ratings radio buttons instead. Something like:
<fieldset>
<label>
<input type="radio" name="rating-group" value="1"/>
<span>1</span>
</label>
<label>
<input type="radio" name="rating-group" value="2"/>
<span>2</span>
</label>
etc
</fieldset>
By doing that you'll get built-in keyboard support. And you won't have to do stuff in the javascript to style them as you can get which button has the :checked state directly in the CSS.
Marked as helpful
@MonicaDalosto
Submitted
Hey Folks, I've just finished my first project using Javascript, and I'd really appreciate any feedback to help me improve my code. I've tried to use a bit of Utility First CSS concepts. What do you think about it? How can I improve or make my code more advanced... I am open to any other suggestions. Thanks in advance!!!
This looks very good and works nicely. The javascript is fairly concise too and readable, which is good.
There is one main thing that you could try which might simplify this a bit.
You can turn this into a form, rather than using buttons with event listeners. This has a few benefits. First it is more semantic so users of assistive technology will get better hints for how to interact with your component. Second, it will allow you to delete a large part of the javascript around styling, as you can use built in HTML and CSS instead.
The html would look something like
<form>
<fieldset>
<legend>Rating</legend>
<label>
<input type="radio" name="rating" value="1">
1
</label>
<label>
<input type="radio" name="rating" value="1">
2
</label>
</fieldset>
<button type="submit">Submit</button>
<form>
Then in your CSS you can access focus, hover and checked states, removing the need to add or remove classes with javascript
input[type="radio"] {
// all the styling for your standard radio buttons
}
input[type="radio"]:hover,
input[type="radio"]:focus-visible {
// the color change for when it's hovered/focused
}
input[type="radio"]:checked {
// the color change for the selected item
}
In your javascript you can then get the selected value when you submit the form
const formElement = document.querySelector('form')
formElement.addEventListener('submit', (event) => {
event.preventDefault() // stops the browser doing a full refresh
const formData = new FormData(form) // extract the data from the form
const rating = formData.get('rating') // get the value of the 'rating' field
// now you can do whatever you want with your rating value
})
Marked as helpful
@Hamza-Noah
Submitted
This looks very good. Well done. There are only a few things I could find that would be worth fixing.
When you shrink the screen down below about 330px, the price and the change link start to overlap each other. If you put flex-wrap: wrap on the container, that will make sure they stay readable if there isn't space to show it all on one line.
When you tab through with a keyboard, the proceed to payment link doesn't have a visible focus indicator. That's because the default indicator is blue and blends in with the button color. You could use outline-offset to space it out from the button by a couple of pixels. Or you could change outline-color to black, to give enough contrast.
The text size on mobile (particularly the price) is a bit small and difficult to read. 11px is very small. I would be very careful with anything under 14px, especially for something as important as price information.
In general, it looks like you built this desktop-first. That's fine, and it works, but it can be easier and more efficient to code mobile-first, and then only make changes in media queries for desktop.
Marked as helpful
@mjbaga
Submitted
This project was done with React, but I wanted to try more tools like Redux for state, Tailwind for styling, Firebase's Firestore for its NoSQL database and Firebase Cloud Storage for images. Worked on the challenge for 7 days and managed to get through it.
Hope this codebase helps fellow developers learning React. Any improvements, feel free to share your comments. Thank you!
Hey there. This looks excellent, and is very fully featured. As far as I can tell the cart and checkout flow work well.
I don't have time to give this as deep a review that I would like, but there are three things that I've spotted that I think would be worth looking at.
Image loading and layout shift. On a lot of the pages (for instance the Headphone category page) the take a long time to load in, and then when they do the whole layout shifts to make space for them. This is quite distracting for a user, and Google will penalize you in search results for it. To solve this you can make sure image placeholders are in place immediately, make sure the assets are optimized and served efficiently so loading is fast, make sure your image elements have width and height attributes so they always take up the same space, even before the asset has been fetched, or wait to load the whole page once everything is loaded.
Form validation. If I miss filling out a field at checkout, the form just tells me something is wrong. It doesn't tell me which field is wrong, or highlight them. This can be pretty confusing. It would be great to highlight the exact errors so a user knows what to fix.
Loading spinners. Navigating between pages takes a long time, and the site just shows a loading spinner in between. For an ecommerce website this isn't ideal. Users will leave you site and go to a competitor if it takes too long to do these things. I haven't looked into the code to see the exact reason for the slow loading. But for these product pages, they aren't different for different users, so instead of fetching the data on the client, you could build them as static pages. A framework like NextJS and their getStaticProps loader may help with this. This is a problem that isn't so important to solve when you're building an app, where users must be logged in to do anything. But for a consumer website, and especially for an ecommerce site, it's vital.
Marked as helpful
@K4UNG
Submitted
I tried to speed run this but failed miserably. Writing html and setting things up alone took almost an hour. But overall, I'm happy with what I got. I tried using as many semantic tags as I could think of. Any kind of feedback or suggestion on how to improve would be highly appreciated. Happy Coding!
This looks great and is nicely responsive. And the theme switcher works well.
In general, I would suggest that trying to rush things is not a very useful thing to do. Doing things well is far more important, and speed just comes with familiarity.
In terms of semantics and accessibility, there are a few things to fix.
The cards have a hover effect and a pointer for the cursor. But there are no interactive elements on them. There is an implication that clicking on this card will do something, but you haven't provided any means to do that (a button or an anchor). So this won't work. And in addition, it won't be accessible for keyboard users. This is an excellent article on how to build proper cards https://inclusive-components.design/cards/
You have incorrect alt text (e.g. the Facebook icon's alt text says Youtube)
Be careful with headings. A heading should be meaningful when read independently of other content. And your headings as a whole should read like a table of contents. Currently, you have headers like <h1>1987</h1>. This is meaningless on its own so doesn't work as a good heading.
Be careful with the level of headings. You should only have one h1 and then every other heading should follow the correct order.
Careful with what screen readers will read. You've used alt text on the arrow icons. So currently they will read something like "Image Up arrow 12 today". That's better than just "12 today" but it would be better to read something like "Up 12 today". So you can make the icons have an empty alt, and use screen-reader-only text to get the right result.
Marked as helpful
@fdsantos300
Submitted
This looks good, though I'd be tempted to add a little padding so the card doesn't quite touch the sides of the screen on small widths. Or let it touch the sides but remove the border-radius in that case.
A note here about semantic tags.
Interactive elements must be either buttons (if they do something like submit a form) or anchors (if they are a link for navigation). Whenever you see a hover-effect in a design, that implies it is an interactive element.
So here, the image and the heading and the username must all be wrapped in interactive elements. Those will then get :hover and :focus or :focus-visible styles in the css.
Thinking about this further still. The design doesn't specify whether these should be buttons or anchors. But if we imagine a real card. Probably when we click on the title we would go to a detail page. And when we click on a username then we'd go to that user's profile. So those should both be in anchor tags. When we click on the image, we might expect a bigger version of the image to open in a lightbox overlay. In which case it should be a button. Or it might navigate to a full page with a large image, in which case it should be an anchor. You can decide which makes most sense there.
Marked as helpful
@MuhammadTatma
Submitted
at first i find it difficult to implement hover in the image, finally i use CSS background-image property and HSLA color. is there any other approach to do that ? did i use correct html tag based on the semantic ? any feedback and suggestions are very welcome !
A note here about semantic tags.
Interactive elements must be either buttons (if they do something like submit a form) or anchors (if they are a link for navigation). Whenever you see a hover-effect in a design, that implies it is an interactive element.
So here, the image and the heading and the username must all be wrapped in interactive elements. Those will then get :hover and :focus or :focus-visible styles in the css.
Thinking about this further still. The design doesn't specify whether these should be buttons or anchors. But if we imagine a real card. Probably when we click on the title we would go to a detail page. And when we click on a username then we'd go to that user's profile. So those should both be in anchor tags. When we click on the image, we might expect a bigger version of the image to open in a lightbox overlay. In which case it should be a button. Or it might navigate to a full page with a large image, in which case it should be an anchor. You can decide which makes most sense there.
Marked as helpful
@MathisHumbert
Submitted
Hi! This is my first MERN project!
So I used MongooDB, Express, React, NodeJS, Redux and Styled Components to code. I learned a lot about React and NodeJs by doing this awesome project! Feel free to comment my code
I would suggest it might be a good idea to either make this work without a login, or provide a demo user for people to use to login with
Some people may not be comfortable with you storing their email address and password, so you could be limiting the number of people that will click through and view your work.
Marked as helpful
