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

expected output of an empty string in spacing properties #6077

Closed
2 tasks done
s-cork opened this issue Apr 13, 2024 · 0 comments · Fixed by #6078
Closed
2 tasks done

expected output of an empty string in spacing properties #6077

s-cork opened this issue Apr 13, 2024 · 0 comments · Fixed by #6078

Comments

@s-cork
Copy link
Contributor

s-cork commented Apr 13, 2024

Dependencies check up

  • I have verified that I use latest version of all @mantine/* packages

What version of @mantine/* packages do you have in package.json?

7.7.2

What package has an issue?

@mantine/core

What framework do you use?

Other, I will specify in the bug description

In which browsers you can reproduce the issue?

All

Describe the bug

I don't think it's intended behvaior
but an empty string for something like mt in the style props leads to the output marginTop: calc(0px * (...))
I would expect an empty string to be treated like undefined

(Similar to how it is in react, and css generally)

If possible, include a link to a codesandbox with a minimal reproduction

(set mt="" on any component and inspect the element)

Possible fix

in createConverter
this line is true for empty strings

      // Number("") -> 0
      if (!Number.isNaN(Number(replaced))) {

so we should explicitly check for empty string

There's also a corollary of this, that if you have extra spaces in your prop, then you get weird things in the output e.g. mt=" 4px"
calc(0rem * var(--mantine-scale)) calc(0.25rem * var(--mantine-scale))

The str.split.join function in the same place could be a bit cleverer
(getting blocks of non-space characters, rather than splitting on spaces)

Self-service

  • I would be willing to implement a fix for this issue
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

Successfully merging a pull request may close this issue.

1 participant