Negative margin not added properly when using `margin` shorthand

[gsaran]

Adding negative margin to Box as <Box margin="-10px 0 10px 0"></Box> results in margin: calc(10px 0 10px 0* -1);; thus evaluating to a Invalid Property Value

If the same margin is divided two props as <Box margin="0 0 10px 0" marginTop="-10px"></Box>, it results in margin: 0 0 10px; margin-top: calc(10px* -1); which renders the element in expected position.

Issue is reproducible in 1.29.0 & also v2

[derekhawker]

It also happens with other margin values. Something about seeing a '-' character in the beginning?

                margin="0 0 10px 0"
                marginLeft="-10px"

Saw the styled-system config for margin passes any input to a transform function https://github.com/trendmicro-frontend/tonic-ui/blob/v2/packages/styled-system/src/config/margin.js#L9

defined here https://github.com/trendmicro-frontend/tonic-ui/blob/v2/packages/styled-system/src/utils/transforms.js#L35

Kind of looks like we should bail out of that positiveOrNegative function if we detect multiple values in the margin/padding shorthands

[trend-albert-shala]

Kind of looks like we should bail out of that positiveOrNegative function if we detect multiple values in the margin/padding shorthands

Yes however there may likely be a use-case where someone passes in multiple tonic values eg: -8x 0 4x 0 which this function will help convert to the proper css values, however, I'm wondering why we're using calc here instead of simply returning -${value} like in the earlier version? I'm not quite seeing the added advantage of using calc, but there might be some benefits or use-cases that I'm missing? Although, the use of calc here not only seems a bit redundant, but it also restricts us from passing our own calc function for margin/padding, which can lead to an invalid property value as well.

[derekhawker]

Yes however there may likely be a use-case where someone passes in multiple tonic values eg: -8x 0 4x 0 which this function will help convert to the proper css values.

yes, though one minor note. margin/padding shorthand doesn't support multiplier values except in the very simple case of margin="8x". Something like margin="8x 0 4x 0" is passed exactly as it is to browser.

I guess that would be a nice thing to fix too, though, I never minded it much. I can never remember what the margin/padding shorthands are and would rather someone set the individual margin properties, instead.

[cheton]

Kind of looks like we should bail out of that positiveOrNegative function if we detect multiple values in the margin/padding shorthands

That's right!

Yes however there may likely be a use-case where someone passes in multiple tonic values eg: -8x 0 4x 0 which this function will help convert to the proper css values, however, I'm wondering why we're using calc here instead of simply returning -${value} like in the earlier version? I'm not quite seeing the added advantage of using calc, but there might be some benefits or use-cases that I'm missing? Although, the use of calc here not only seems a bit redundant, but it also restricts us from passing our own calc function for margin/padding, which can lead to an invalid property value as well.

The calc function is required when useCSSvariables is enabled in the theme config. It automatically converts theme tokens into CSS variables, as shown below:

Note: You cannot add a negative symbol in front of a CSS variable, such as margin-top: -var(--tonic-space-1x), in this case. See https://stackoverflow.com/questions/49469344/using-negative-css-custom-properties

I realize this restricts us from passing a custom calc function. I will investigate a better way to detect multiple values so that the negation is applied only to theme tokens. PRs are also welcome for the fix.

[cheton]

CSS Variables

Regarding CSS variables, you can manually enable this by setting useCSSVariables to true in the theme config:

https://trendmicro-frontend.github.io/tonic-ui/react/v2/getting-started/usage

import React from 'react';
import {
  Box,
  TonicProvider,
  colorStyle, // [optional] Required only for customizing color styles
  theme, // [optional] Required only for customizing the theme
} from '@tonic-ui/react';

// Enable CSS variables
theme.config.useCSSVariables = true;

function App(props) {
  return (
    <TonicProvider
      colorMode={{
        defaultValue: 'dark', // One of: 'dark', 'light'
      }}
      colorStyle={{
        defaultValue: colorStyle, // Custom color style
      }}
      theme={theme}
      useCSSBaseline={true} // If `true`, apply CSS reset and base styles
    >
      <Box {...props} />
    </TonicProvider>
  );
}

[cheton]

A PR was created to resolve this issue: https://github.com/trendmicro-frontend/tonic-ui/pull/869

https://github.com/trendmicro-frontend/tonic-ui/blob/254b31455ccfcfdc669e4b8c19d02e4921d92aea/packages/styled-system/src/utils/transforms.js

  if (typeof value === 'string') {
    const absoluteValue = (value.startsWith('+') || value.startsWith('-')) ? value.slice(1) : value;
    const isNonNegative = !value.startsWith('-');

    // Return the result if the value is non-negative or if the scale object does not contain the absolute value
    if (isNonNegative || !Object.prototype.hasOwnProperty.call(scale, absoluteValue)) {
      return getter(scale, value, options);
    }

    const n = getter(scale, absoluteValue, options);

    // Handle CSS variables for negative values
    if (useCSSVariables && isSimpleCSSVariable(n)) {
      // https://stackoverflow.com/questions/49469344/using-negative-css-custom-properties
      return `calc(0 - ${n})`;
    }

    // Handle numeric value
    if (typeof n === 'number' && Number.isFinite(value)) {
      return n * -1;
    }

    return `-${n}`;
  }

[cheton]

This issue was fixed in 1.29.3 and 2.0.1