mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.6k stars 32.21k forks source link

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

Open edazpotato opened 2 years ago

edazpotato commented 2 years ago

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.

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.

mnajdova commented 2 years ago

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 :)

timothyarmes commented 2 years ago

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

edazpotato commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

jankanty commented 2 years ago

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

cristi-shoreline commented 2 years ago

@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 commented 2 years ago

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

jankanty commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

zif002 commented 2 years ago

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 commented 2 years ago

Please read through the initial issue https://github.com/mui/material-ui/issues/16205#issuecomment-504116608 and the PR that changed the behavior https://github.com/mui/material-ui/pull/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 commented 2 years ago

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 commented 1 year ago

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 commented 7 months ago

Any update on this?

ZeeshanTamboli commented 7 months ago

Feel free to create a PR if anyone wants to.

HansDP commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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.