[Grid] Fix generated classes for spacing prop when the value is object

[jayeclark]

Signed-off-by: Jay Clark jay@jayeclark.dev

Fixes #29861

Fixes the issue with the class name(s) not parsing correctly when spacing is an object instead of a number or string... also added some additional tests for Grid & some logic to cover another common error that I noticed - if spacing is left undefined, the class gets parsed as MuiGrid-spacing-xs-undefined)

• I have followed (at least) the PR section of the contributing guide.

[mnajdova]

Suggested change

	container && spacing?.xs && spacing.xs !== 0 && styles[`spacing-xs-${String(spacing.xs)}`],
	getSpacingStyles(styles, container, 'xs);,

Same when extracting the classes below.

Would be great if we can simplify this to something like:

[jayeclark]

Sure, good idea. I’ll work on simplifying.

[jayeclark]

@mnajdova I added an additional commit that implements this idea.

[Janpot]

Should it rather be like this?

Suggested change

	typeof +spacing === 'number' &&
	typeof spacing === 'number' &&

because

// always evaluates to `true`
typeof +spacing === 'number'

[jayeclark]

I see my mistake (not the first time I’ve been tripped up by NaN) - this is the condition I was trying to write:

isNaN(Number(spacing))

It will only evaluate to true if spacing is already a number or if spacing can be converted to a valid number.

The spacing property can be a string, number, or object. Strings that can be converted to valid numbers should be accepted and processed as if they are numbers. Other strings should be ignored.

[jayeclark]

@Janpot I added an additional commit that corrects this error.

[Janpot]

Suggested change

spacing !== 0 &&

This is redundant with the line before

[jayeclark]

I don’t believe it’s redundant, if a variable exists but is undefined, the following condition evaluates to true:
myvariable !== 0

Having only spacing !== 0 as the condition was what was causing the class to parse as MuiGrud-spacing-undefined when spacing wasn’t specified.

Should zero values for spacing be parsed into a class? I was under the impression from the original code that they should not be. (This made sense to me since the default spacing is zero, so, no need to specify it.)

That said, maybe the condition should be changed to
Number(spacing) !== 0
to cover a user entering spacing=“0” (ie zero spacing but written as a string.)

[jayeclark]

Sorry, am on my phone & misread which part of the condition was being suggested to be cut.

Yes, having just spacing as the condition will work for all cases except spacing=“0” - and my code as written wouldn’t handle that case correctly anyway.

I think my revised version would, but you’d probably know better whether that’s something that should be addressed or not.

[jayeclark]

@Janpot I added an additional commit that addresses this.

[hbjORbj]

@jayeclark Thanks for creating the PR. Your changes are currently breaking two tests for Grid. Please run yarn test:unit --grep Grid to make sure all tests pass.

[mui-pr-bot]

Details of bundle changes

@material-ui/core: parsed: +0.11% , gzip: +0.16%
@material-ui/lab: parsed: +0.15% , gzip: +0.14%

Generated by 🚫 dangerJS against db3fa6c

[jayeclark]

@hbjORbj Added an additional commit, and the unit tests are passing on my end now... (But, they were also passing locally when I originally submitted the PR, so, I'm not sure what's up.)

.

[hbjORbj]

@jayeclark Thanks for your contribution! I refactored your code a bit so please feel free to have a look and correct me if you disagree. Other than that, this looks good to me!

Before:

• code sandbox: https://codesandbox.io/s/grid-spacing-bug-s9hp9?file=/demo.js

After:

• code sandbox: https://codesandbox.io/s/grid-spacing-bug-forked-ofw4w?file=/demo.js

cc @Janpot for another pair of eyes :)

[jayeclark]

Thanks @hbjORbj , most of this makes sense to me.

I have two concerns:

1. Line 185… if spacing = ‘three’, typeof spacing === ‘string’ && typeof Number(spacing) === ‘number’ will resolve to true (NaN is a number.) I think Number.isNaN(Number(spacing)) === false (or !Number.isNaN(+spacing) if that simplified style is better) needs to stay in the condition. Probably I also need to add a better written test for this, because your code changes theoretically should have broken one of the Grid tests.

2. Are we absolutely certain that the value of spacing that is passed to resolveSpacingClasses will never be undefined? If it is undefined, spacing <= 0 will resolve to false and the code will continue to line 191, at which point I believe the code will break (attempting to destructure an undefined variable breaks the code - whereas attempting to destructure a non-object variable creates the desired variables and sets them to undefined.)

IIRC there is a line elsewhere in the code that sets spacing to 0 if undefined, but I’m just wondering if future changes might eliminate that - or if there is some way (through incompetence/unfamiliarity with the library) to make the value of spacing currently passed to resolveSpacingClasses() be an undefined value. If this happens, the resulting error message might be hard for a newer user to understand.

Maybe I’m being overly nit-picky? I guess one easy way to future proof the function on item 2 while keeping most of your changes (which I do think make it more readable) would be to add a default value for spacing in the resolveSpacingClasses() function declaration?

[jayeclark]

@hbjORbj Please ignore my last comment, I just saw the second commit :)

[hbjORbj]

Please ignore my last comment, I just saw the second commit :)

@jayeclark I am sure your first comment is no longer an issue too? :) I use this condition typeof spacing === 'string' && !Number.isNaN(Number(spacing))

[jayeclark]

@hbjORbj Yes, sorry for the unclear language, both of my initial concerns were resolved by the second commit!