Skip to content

Instantly share code, notes, and snippets.

@marlalain
Last active March 12, 2024 17:53
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save marlalain/be3b4d1c5f373bd1acc0aa99240eccf6 to your computer and use it in GitHub Desktop.
Save marlalain/be3b4d1c5f373bd1acc0aa99240eccf6 to your computer and use it in GitHub Desktop.

The Problem

Adding inner margins and padding to one component should be avoided. In one screen it may make sense to do this, but as soon as other screen doesn't require that margin, the component becomes incorrect. Do not create new components with inner margins. There are a few ways to undo this. Let's first create an incorrect example:

❌ Wrong

type ProfileCardItemProps = {
  label: string
  value: string
}

function ProfileCardItem({ label, value }: ProfileCardItemProps) {
  return (
    <GridItem marginTop={3}>
      <Text
        wordBreak="break-word"
        fontWeight={500}
        fontSize={{ base: 'sm', lg: 'md' }}
        color="gray.800"
      >
        {label}
      </Text>

      <Text wordBreak="break-word" fontWeight={600} fontSize="xl" color="gray.900">
        {value}
      </Text>
    </GridItem>
  )
}

This may seem fine at first. This could be adding some margin between this component and another section, or even adding margin between items in a list. It accomplishes its goal. But what happens if I try to add this to another screen? The margin will still be there. Someone may be inclined to fix this issue by doing this:

❌ Wrong

type ProfileCardItemProps = {
  label: string
  value: string
  marginTop?: string | number
}

function ProfileCardItem({ label, value, marginTop = 3 }: ProfileCardItemProps) {
  return (
    <GridItem marginTop={marginTop}>
      <Text
        wordBreak="break-word"
        fontWeight={500}
        fontSize={{ base: 'sm', lg: 'md' }}
        color="gray.800"
      >
        {label}
      </Text>

      <Text wordBreak="break-word" fontWeight={600} fontSize="xl" color="gray.900">
        {value}
      </Text>
    </GridItem>
  )
}

This code does not scale. Try to imagine a few months of development like this. Picture how this component will look like if every prop that this component needs requires another property being added to ProfileCardItemProps. It will eventually be HUGE, right? So what can we do instead?

The Solution

Components should be responsible for what they do and what they are. A <TextInput> component shouldn't have props that manipulate the wrapper container it has. The general rule for a component should be to occupy as much space as it can, and be flexible in size for other screens. If you think it makes sense for the component to have a wrapper that manipulate something around the component, instead of changing how you write this component, you will change how you use it.

✅ Correct

export default function Page() {
  return (
    <Box marginTop={3}>
      <ProfileCardItem label="Hello" value="World">
    </Box>
  )
}


function ProfileCardItem({ label, value }: ProfileCardItemProps) {
  return (
    <Flex direction="column">
      <Text
        wordBreak="break-word"
        fontWeight={500}
        fontSize={{ base: 'sm', lg: 'md' }}
        color="gray.800"
      >
        {label}
      </Text>

      <Text wordBreak="break-word" fontWeight={600} fontSize="xl" color="gray.900">
        {value}
      </Text>
    </Flex>
  )
}

Making it as "dumb" as you can will allow it to be as flexible as it could.

This post was written with the help of @santospatrick and @brunoeduardodev.

@brunoeduardodev
Copy link

✅ LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment