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

[system] Allow theme.spacing() to return a number #29086

Open
1 task done
edazpotato opened this issue Oct 16, 2021 · 21 comments
Open
1 task done

[system] Allow theme.spacing() to return a number #29086

edazpotato opened this issue Oct 16, 2021 · 21 comments
Labels
new feature New feature or request package: system Specific to @mui/system

Comments

@edazpotato
Copy link

edazpotato commented Oct 16, 2021

Motivation 🔦

I love the spacing utility function because it makes keeping spacing consistent throughout an application very easy, but in some cases, I want to tweak the value returned by it a little. This is currently quite difficult because the function always returns a string with "px" appended to it.
Currently, if I want to, for example, add 2 to the return value of the spacing function, I would have to do something like this:

`${Number(theme.spacing(3).slice(0, -2)) + 2}px`

I did check to see if the spacing value provided to createTheme() was exposed anywhere in the Theme object, but it didn't look like it was.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

There should be some way (possibly a fifth argument?) to make the spacing function return a number. If this isn't possible, then the spacing multiplier value provided to createTheme() should be exposed somewhere in the Theme object so that developers can use it to do the multiplication themselves.

@edazpotato edazpotato added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 16, 2021
@mnajdova
Copy link
Member

As per your code this is all it takes to implement it: Number(theme.spacing(3).slice(0, -2)) It should be fairly easy to add this inf your custom theme, or even as a util:

const getSpacing = (theme, value) => Number(theme.spacing(value).slice(0, -2))

I am not sure if it is worth adding it to the theme. I would wait to see if it will get some traction :)

@mnajdova mnajdova added new feature New feature or request package: system Specific to @mui/system and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 20, 2021
@timothyarmes
Copy link

I would also like access to the raw spacing value. Having to convert from a string to a number is a little excessive.

@edazpotato
Copy link
Author

If I recall correctly, this used to be the default behaviour of theme.spacing() in V4, and it only began returning strings with appended px units in V5.

@jankanty
Copy link

Yep. After migration all my calculated paddings are broken. Issue: I have <Card /> with custom padding, but if specific card is "selected" it has border which i need to subtract from my padding which was previously pure spacing(). Now it throws everywhere NaNpx

@chaosmirage
Copy link
Contributor

Workaround: calc(${theme.spacing(2)} - 1px)

@jankanty
Copy link

Nope. It's not workaround. It force browser to calc thing it shouldn't. Workaround is parseInt(theme.spacing(2)) - 1

@cristi-shoreline
Copy link

@jankanty Could you please explain why you're subtracting 1?

Anyway, we're migrating now from v4 to v5 and I'd also like to have the spacing utility to return numbers. It's really useful when having to calculate padding.

We're working with tons of charts and use the spacing utility of the theme to make sure everything's aligned and for this, we calculate extra spacing using offsets to position SVG elements correctly. For every single usage, we have to parseInt the string value.

The suggestion of adding a second param for the (value: number): string; signature would be awesome.

@jankanty
Copy link

@cristi-shoreline it was only for example for previous person

@jankanty
Copy link

I really hope it would be fixed to previous behaviour so parseInt is best workaround since you can after that quickly remove it with search and replace.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Feb 10, 2022

I was working on this. But found out that can't we simply supply a decimal value accordingly to theme.spacing?
Eg:

`${Number(theme.spacing(3).slice(0, -2)) + 2}px` == 26px

We can do,

theme.spacing(3.25) == 26px

cc: @mnajdova

@mnajdova
Copy link
Member

Sorry, I don't understand what you mean we cannot apply decimal value.

@zif002
Copy link

zif002 commented Feb 17, 2022

In version 4 it worked correctly if I needed a number, it returned a number, if I needed something in px, it returned a string in px, please return that behavior

@mnajdova
Copy link
Member

Please read through the initial issue #16205 (comment) and the PR that changed the behavior #22552 if you are interested why the change was made. There is a simple fix to the solution, provided in the comments above. I don't think it's worth spending time on this issue.

@paul23-git
Copy link

I know the "solution" works, but that's really brittle: who is to say that the units will forever be "px". The other solution by using calc is more robust - however it doesn't work in all places, notably things like breakpoints don't allow such syntax.

@ochicf
Copy link

ochicf commented Dec 19, 2022

I just wrote this in a MUI v5 project to easily use this version of the function.

In a folder called theme-augmentations/spacingV4 or something similar:

import { Theme } from "@mui/material";
import { SpacingArgument } from "@mui/system/createTheme/createSpacing";

/**
 * Version of the `spacing` function that returns a number or an array of numbers
 * instead of a string. Suffixed with `V4` because it's the behaviour it had on @mui/v4.
 * 
 * @see https://mui.com/material-ui/customization/spacing/
 * @see https://github.com/mui/material-ui/issues/29086
 */
export interface SpacingV4 {
  (): number;
  (value: number): number;
  (topBottom: SpacingArgument, rightLeft: SpacingArgument): [number, number];
  (top: SpacingArgument, rightLeft: SpacingArgument, bottom: SpacingArgument): [
    number,
    number,
    number
  ];
  (
    top: SpacingArgument,
    right: SpacingArgument,
    bottom: SpacingArgument,
    left: SpacingArgument
  ): [number, number, number, number];
}

export interface WithSpacingV4 {
  spacingV4: SpacingV4;
}

export default function spacingV4(
  /* mutable */ theme: Theme
): Theme & WithSpacingV4 {
  return Object.assign(theme, { spacingV4: makeSpacingV4(theme) });
}

export const makeSpacingV4 = (theme: Theme): SpacingV4 => {
  // @ts-expect-error: wrongly typed from the `SpacingV4` interface
  const spacingV4: SpacingV4 = (...args: number[]) => {
    const [top, right, bottom, left] = theme
      // @ts-expect-error: wrongly typed from `number[]` to tuples
      .spacing(...args)
      .split(" ")
      .map((value) => Number.parseInt(value, 10));

    switch (args.length) {
      case 0:
      case 1:
        return top;
      case 2:
        return [top, right];
      case 3:
        return [top, right, bottom];
      case 4:
        return [top, right, bottom, left];
    }
  };

  return spacingV4;
};

Then in the theme.ts:

import spacingV4, { SpacingV4 } from "./theme-augmentations/spacingV4";

const theme = createTheme();

export default spacingV4(theme);

declare module "@mui/material/styles" {
  interface Theme {
    spacingV4: SpacingV4;
  }
}

And then wherever the theme is available:

theme.spacingV4(); // 8
theme.spacingV4(2); // 16
theme.spacingV4(2, 3); // [16, 24]
theme.spacingV4(2, 3, 4); // [16, 24, 32]
theme.spacingV4(2, 3, 4, 5); // [16, 24, 32, 40]

@michaelmontero
Copy link

Any update on this?

@ZeeshanTamboli
Copy link
Member

Feel free to create a PR if anyone wants to.

@HansDP
Copy link

HansDP commented May 21, 2024

Hi

I'm using the Joy UI library and I do have the same request. I want to use the spacing value during painting on a canvas, to align the custom graphical rendering with the spacing used in the HTML.

I'm in the process of updating the Joy UI package to its latest version:
@mui/joy: from 5.0.0-beta.32 to 5.0.0-beta.36

In the previous version, I could get the spacing value in pixels by using the parseInt work-around:

const MyComponent = () => {
  const theme = useTheme()
  const spacing = parseInt(theme.spacing(), 10)

  return <div>{spacing}</div> // -> outputs "8px"
}

const App = () => {
  return (
    <ThemeProvider ...>
      <MyComponent />
    </ThemeProvider>
  )
}

Although a bit of overhead, but it did the job.
Since the update to the latest version (5.0.0-beta.36), this approach does no longer work. The theme.spacing() call now returns var(--joy-spacing).

And thus I loose the possibility to get the actual spacing-value.

const MyComponent = () => {
  const theme = useTheme()
  const spacing = parseInt(theme.spacing(), 10)

  return <div>{spacing}</div> // -> outputs "var(--joy-spacing)"
}

const App = () => {
  return (
    <ThemeProvider ...>
      <MyComponent />
    </ThemeProvider>
  )
}

Is there any supported solution for this?

Regards
Hans

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented May 22, 2024

@HansDP I think it was due to the change done in #40224. Maybe you can use window.getComputedStyle() method?

const useSpacingValue = () => {
  const [spacingValue, setSpacingValue] = useState(0);

  useEffect(() => {
    const computedStyle = getComputedStyle(document.documentElement);
    const spacing = computedStyle.getPropertyValue('--joy-spacing');
    const spacingInPixels = parseInt(spacing, 10);
    setSpacingValue(spacingInPixels);
  }, []);

  return spacingValue;
};

const MyComponent = () => {
  const spacing = useSpacingValue();

  return <div>{spacing}</div>; // This should output the spacing value in pixels
};

Please create a new issue if this doesn't work. This is unrelated to this issue.

@HansDP
Copy link

HansDP commented May 22, 2024

@ZeeshanTamboli Thanks for the suggestion. I already considered that, but rejected it as a good solution.

The getComputedStyle solution requires a JavaScript variable to be transferred into the DOM (using CSS) to be able to extract it back into JavaScript. This seems like an anti-pattern and using the wrong solution.

Secondly, this requires an additional render cycle (the first render cycle, the spacing variable will be 0, the second render cycle will contain the correct value).

I do think this is related to the issue. As stated by the OP, the request is to get the spacing multiplier from the theme. This is exactly what I need (something that was possible using a work-around in the past, but in the end, I do want to get a hold of the spacing multiplier). But, if creating a new ticket is what is needed here, just let me know. I gladly create a duplicate ticket.

@ZeeshanTamboli
Copy link
Member

@HansDP I think it would be better to also create a new ticket as it is related to recent changes. This issue is slightly similar but it is for returning a number without a unit. Now it returns CSS variable.

I think the change done here for the test is incorrect.

@danilo-leal danilo-leal changed the title Allow theme.spacing() to return a number [system] Allow theme.spacing() to return a number May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system
Projects
None yet
Development

No branches or pull requests