Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Rule Proposal]: Use Value with Unit #20

Open
Tracked by #16
no-yan opened this issue Jan 30, 2022 · 9 comments
Open
Tracked by #16

[New Rule Proposal]: Use Value with Unit #20

no-yan opened this issue Jan 30, 2022 · 9 comments
Assignees

Comments

@no-yan
Copy link
Collaborator

no-yan commented Jan 30, 2022

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 no-yan changed the title use-unit-value [New Rule Proposal]: Use Unit Value Jan 30, 2022
@no-yan
Copy link
Collaborator Author

no-yan commented Jan 30, 2022

@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 no-yan changed the title [New Rule Proposal]: Use Unit Value [New Rule Proposal]: Use Value with Unit Jan 30, 2022
@yukukotani
Copy link
Owner

@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")[],
}

@no-yan
Copy link
Collaborator Author

no-yan commented Jan 30, 2022

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.

@yukukotani
Copy link
Owner

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

@no-yan no-yan self-assigned this Feb 6, 2022
@no-yan
Copy link
Collaborator Author

no-yan commented Feb 11, 2022

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.

@yukukotani
Copy link
Owner

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

@no-yan
Copy link
Collaborator Author

no-yan commented Feb 11, 2022

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

@yukukotani
Copy link
Owner

@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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants