eslint-plugin-chakra-ui icon indicating copy to clipboard operation
eslint-plugin-chakra-ui copied to clipboard

[New Rule Proposal]: Use Value with Unit

Open no-yan opened this issue 3 years ago • 9 comments

Rule Summary

When non-united number is not predefined in theme, suggest adding unit for explicitness.

Example of incorrect code for this rule:

import { Box } from "@chakra-ui/react";

<Box p="11" mx="17" width="200">
  Hello
</Box>;

Example of correct code for this rule:

import { Box } from "@chakra-ui/react";

<Box p="11px" mx="17px" width="200px">
  Hello
</Box>;

It may be desirable to have this rule turned off by default.

Why needed?

In Chakra UI, we can use numbers for spacing without units. However, Chakra UI doesn't offer consistent behavior, and there are times when it is preferable to explicitly add units.

Inconsistency 1: Different conversion methods depending value is predefined or not.

First of all, the behavior differs depending on whether the value has already been defined.

  • let x is 20, then output is 5rem, it will be equal to 80px in most case.
  • let x is 21, then output is 21px

Chakra UI claims mental model of spacing:

Mental model: If you need a spacing of 40px, divide it by 4. That'll give you 10. Then use it in your component.

But it is not true model. Most numbers will not be quadruple.

For example, if you pass a variable to width, some values are quadrupled, some are not. This is something that many developers would not expect.

Inconsistency 2: Some prop does not convert numbers as ${number}px

Secondly, in some case width props can output numbers without px or any units. ref: Different width depending on how props are passed

  • <Box w='11'/> // output will be 11
  • <Box w={11}/> // output will be 11px

I think both of these problems should be improved in Chakra UI itself, but I don't have much motivation to do so right now.

no-yan avatar Jan 30 '22 07:01 no-yan

@Monchi what do you think? And if you think it's worth implementing, do you think it's better to avoid mixing p={12} and w="12px"? I mean,

  • Mixing <Box w="11px" h="12">

  • Not mixing <Box w="11px" h="48px">

no-yan avatar Jan 30 '22 08:01 no-yan

@no-yan Totally agree. I can't believe the second inconsistency...

And if you think it's worth implementing, do you think it's better to avoid mixing p={12} and w="12px"?

I think we can prohibit number values since there is no merit except a very little reduction of bundle size.

Also I prefer the strict rule which forces the use of the single unit not only in the same element but in the whole application. How about providing options like below?

type Option = {
  scope: "wholeApp" | "element",
  allowedUnits: ("px" | "rem")[],
}

yukukotani avatar Jan 30 '22 12:01 yukukotani

Thank you. I'm glad to hear it.

Also I prefer the strict rule which forces the use of the single unit not only in the same element but in the whole application. How about providing options like below?

LGTM!

Now let's implement it. I'll work on it after #21, but I don't mind if you do.

no-yan avatar Jan 30 '22 12:01 no-yan

@no-yan Ok, so please assign you to the issue when getting to work on it to avoid double work!

yukukotani avatar Jan 30 '22 13:01 yukukotani

I've made the prototype of the implementation, but I'm not sure which attribute is the one that receives the spacing value.

I will try to determine it by theme key from https://chakra-ui.com/docs/features/style-props. Column "Theme Key" seems to represent internal handling, and "space" correspond to "spacing" defined in theme.

If you have another idea, please let me know.

no-yan avatar Feb 11 '22 17:02 no-yan

@no-yan Maybe you can refer this object but i'm not sure

yukukotani avatar Feb 11 '22 18:02 yukukotani

This does not handle width etc, so the other objects are also involved? I will look it closer tomorrow.

no-yan avatar Feb 11 '22 20:02 no-yan

@no-yan

First, chakra calls t.spaceT for margin here.

Then, t.spaceT calls toConfig("space", transforms.px) here.

space is ThemeScale that represents "Theme Key" in the doc, and transforms.px is a transformer to convert unitless value to px value.

You can see the same thing for width here.

So in summary, we can refer to ThemeScale like space.margin.scale or layout.width.scale to know which prop is treated as a spacing value.

yukukotani avatar Feb 12 '22 05:02 yukukotani

btw width is t.sizesT while height is t.sizes. This is the cause of your confusion 😂

yukukotani avatar Feb 12 '22 05:02 yukukotani