-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[Grid] Fix generated classes for spacing
prop when the value is object
#29880
Conversation
Signed-off-by: Jay Clark <jay@jayeclark.dev>
Signed-off-by: Jay Clark <jay@jayeclark.dev>
spacing !== 0 && | ||
typeof +spacing === 'number' && | ||
styles[`spacing-xs-${String(spacing)}`], | ||
container && spacing?.xs && spacing.xs !== 0 && styles[`spacing-xs-${String(spacing.xs)}`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, good idea. I’ll work on simplifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova I added an additional commit that implements this idea.
container && | ||
spacing && | ||
spacing !== 0 && | ||
typeof +spacing === 'number' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it rather be like this?
typeof +spacing === 'number' && | |
typeof spacing === 'number' && |
because
// always evaluates to `true`
typeof +spacing === 'number'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Janpot I added an additional commit that corrects this error.
container && spacing !== 0 && styles[`spacing-xs-${String(spacing)}`], | ||
container && | ||
spacing && | ||
spacing !== 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing !== 0 && |
This is redundant with the line before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Janpot I added an additional commit that addresses this.
spacing
prop when the value is object
@jayeclark Thanks for creating the PR. Your changes are currently breaking two tests for |
@material-ui/core: parsed: +0.11% , gzip: +0.16% |
Signed-off-by: Jay Clark <jay@jayeclark.dev>
aec65cf
to
67fe9df
Compare
@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.) . |
Signed-off-by: Jay Clark <jay@jayeclark.dev>
@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: After: cc @Janpot for another pair of eyes :) |
6538214
to
d7069b2
Compare
d7069b2
to
bf9b5a9
Compare
Thanks @hbjORbj , most of this makes sense to me. I have two concerns:
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? |
@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 |
@hbjORbj Yes, sorry for the unclear language, both of my initial concerns were resolved by the second commit! |
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 asMuiGrid-spacing-xs-undefined
)