Skip to content
OverviewSolutions
13
Comments
119

Alex

@AlexKMarshallLondon2,010 points

I'm a front end developer 🏢 @Qogita, instructor 👨‍🏫 @Codeworks and mentor 🦸‍♂️ @codebar.io. In a former life I was product manager and customer advocate, so I'm especially passionate about awesome functional, accessible and well-tested UIs.

Latest solutions

View more solutions

Latest comments

  • Annas-khan•120
    @Annas-khan
    What are you most proud of, and what would you do differently next time?

    I was able to code most of it on my own didn't need much help.

    What challenges did you encounter, and how did you overcome them?

    Challenges faced -

    1. not able to quite spread the image in the div container as shown in the preview photos
    2. Had problem commiting changes in github
    What specific areas of your project would you like help with?

    with the css part

    QR code page using Flexbox

    3
    Alex•2,010
    @AlexKMarshall

    Ok, lets break this down into HTML and CSS, HTML first

    • Every site must have a <main> element, which contains all the main content for the page, but excludes content that repeats across multiple pages, such as the site header, footer and main navigation.
      • As this is is a single component challenge, and doesn't have any headers, the whole component should be inside <main>. You have a div with a class of main, but a div on its own is not a semantic element, so replace that with <main>
    • You have unnecessary elements. This challenge only really needs (inside of <main>) a single div for the card component itself, and then an <img> a <h2> and a <p> inside that. All the extra wrapping divs are unnecessary and cause extra noise and styling difficulties
      • Always try and use the minimum number of elements, only add any extra ones if you really need them
    • the <br> is especially unnecessary because it's causing the styles to break too. There's almost never a need to use the <br> element and definitely not in this challenge

    Now the CSS

    • The main problem here is inappropriate units. You generally want to avoid vh or vw as it gives you very little control. For instance what does 83vh mean? 83% of the height of the screen - but how high is that? Your screen is likely different to mine, everyone's is different. So is that an appropriate size? Unlikely
      • The best thing to do is not provide a size at all. The browser will calculate sizes automatically, and they will always be responsive. This is the case for heights. You never want to specify a height. What would happen if a content editor decided the paragraph was going to be twice as long? Your component would now be too short and would break. So, instead, always let the browser calculate height.
    • But, sometimes we have to give some indication of size to match the design. Here, if we didn't do something about the width, the component would stretch the whole width of the screen. And on large screens we don't want that. But, we shouldn't set width - again, if we set a value for that, but the user's screen is smaller, the component will overflow the screen and break.
      • So, instead set a max-width (and make sure it's in rem). That way, the component doesn't get too big on large screens, but can still shrink smaller on small ones
    • Centering the component on the screen. You've almost got that right - you have display: flex; align-items: center; justify-content: center; - that's good. That gets your component centered horizontally. And it would center it vertically too, but you'll notice it didn't, and so you've tried to correct for that with some top margin. But, that was the wrong fix
      • The reason it wasn't centered vertically is because the height of the page only takes up as much space as it needs for the content, so there's no extra height for it to center in. So, you need to increase the height somehow. This is one of the only use cases for vh units. You need the page to be at least 100vh. So set min-height: 100vh on <main>. But make sure it's min-height not height. Otherwise, content may get cut off on very small screens. You want the height to be able to expand bigger than the screen if it needs to
    • Font size. This must never be in px. Users must be allowed to change the browser font-size in their settings. Many people need larger fonts to make it easier to read. If you set fonts in px you remove their ability to do this. So, instead always use rem
    • Always use a CSS reset at the start of your CSS. There are a few browser defaults that are unhelpful when styling sites. A rest will correct some of these to more sensible values. Particularly for images. You can use this one https://www.joshwcomeau.com/css/custom-css-reset/
    • The difference between margin and padding. Margin is for a space between one element and its sibling. Padding is for the space between the border of an element and its children.
      • So, the gap between the heading and the paragraph should be margin, not padding
      • And the card container should have some padding (because right now, the text can touch the edge of the card, and it shouldn't)
    Marked as helpful
  • Alex•2,010
    @AlexKMarshall

    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:

    • Rely on inference where possible, it's less to type and avoids the possibility of lying to the compiler. Some particular cases of this I saw were in array map functions. You shouldn't need to add types to the callback, they should already be inferred. If they aren't, it's because the original array hasn't been typed correctly, and that's where the fix should be
    • Keep types close to where they are used. This is a bit more of a preference thing, but having a 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:

    • Avoid too many props. Props aren't bad, but, if you're finding you have to have 5, 10, even 20 props to a component, especially if some of those props seem unrelated to each other, I think that's an indication that using composition would be preferable
    • Keep hooks near the components that use them. This is the same as my advice for the types. Things like the 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.
    • Don't make components do too much. The 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)
    • Be careful with data-driven ui. I like the idea behind the 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
  • Alex•2,010
    @AlexKMarshall

    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:

    Composition

    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>

    Tailwind class strings

    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.

    Visually hidden

    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
  • Alex•2,010
    @AlexKMarshall

    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.

    • Never set width or height on the body. You've set both of them: 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.
    • You will probably want a 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 problem
    • Don't use width 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.
    • Use a CSS reset. It's not super important for this challenge, but having at least 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.
    • Make sure to understand the difference between padding and margin. Padding is the space between the edges of a box and its content. Margin is the space outside an element. Here you need padding on the <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.
    • Don't set spacing in percent. You have things like 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.
    • You have unnecessary spans inside your heading. I suspect you've done that to force a line-break. Don't do that, let the lines break where they naturally do given the space they have available.
    • Your heading should be a <h1>. Headings must go in order like a table of contents.
    • You'll want your image to be display: block to avoid the line height issues caused by the default display: inline. This would generally form part of the CSS reset
    • font-size: 1.08rem seems an odd choice. 1rem would be fine (and is the default)
    • To reiterate, remove the width: 80% on things like the paragraph and the attribution. It's unnecessary, the spacing should be determined by the padding on the parent.
    • Install 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.
  • Alex•2,010
    @AlexKMarshall

    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
  • Alex•2,010
    @AlexKMarshall

    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 */
}
View more comments