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

Validate bare values #13245

Merged
merged 7 commits into from
Mar 15, 2024
Merged

Validate bare values #13245

merged 7 commits into from
Mar 15, 2024

Conversation

RobinMalfait
Copy link
Member

This PR validates incoming bare values to ensure we generate CSS that makes sense.

Before this PR, we would generate values such as:

.z-index {
  z-index: index;
}

.flex-foo {
  flex: foo;
}

Which is invalid, after this PR, these vaues would not be generated at all.

@RobinMalfait RobinMalfait changed the title Validate incoming bare values Validate bare values Mar 14, 2024
@RobinMalfait RobinMalfait force-pushed the fix/validate-invalid-bare-utilities branch from f2120ec to 8c06f5e Compare March 14, 2024 12:19
@@ -628,7 +634,10 @@ export function createUtilities(theme: Theme) {
staticUtility('col-span-full', [['grid-column', '1 / -1']])
functionalUtility('col-span', {
themeKeys: [],
handleBareValue: ({ value }) => value,
handleBareValue: ({ value }) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the validation for col-span, col-start, col-end, row-span, row-start, and row-end in a separate commit because they can technically almost contain any value. If a "name" is used, then they would refer to named grid areas: https://developer.mozilla.org/en-US/docs/Web/CSS/grid-column-start

/* <custom-ident> value */
grid-column-start: somegridarea;

/* <integer> + <custom-ident> values */
grid-column-start: 2;
grid-column-start: somegridarea 4;

So the question is, do we want to allow anything here, or is it fine to only accept integer values for bare values, e.g.: col-span-123 and if you want to generate values using names that you should use arbitrary values instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda think just do numbers and use arbitrary values for other stuff 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I think bare values should just be numbers. Feels much simpler to explain that too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright perfect, than I can keep the commit in there, nice 👍

@thecrypticace
Copy link
Contributor

This looks good. I'm just thinking out loud but I wonder if we can lift this check up a level to before we call handleBareValue? The only oddball out then is aspect-* in which the bare value is {number}/{number} but I'm not sure how we'd want to handle that. Any ideas?

@RobinMalfait
Copy link
Member Author

This looks good. I'm just thinking out loud but I wonder if we can lift this check up a level to before we call handleBareValue? The only oddball out then is aspect-* in which the bare value is {number}/{number} but I'm not sure how we'd want to handle that. Any ideas?

I initially added a validateBareValue function, but then noticed that we already have some validation happening here:

handleBareValue: ({ value }) => {
if (!value.endsWith('%')) return null
let num = Number(value.slice(0, -1))
// Only 50-200% (inclusive) are valid:
// https://developer.mozilla.org/en-US/docs/Web/CSS/font-stretch#percentage
if (Number.isNaN(num) || num < 50 || num > 200) return null
return value
},

therefor I inlined it in the handleBareValue function.

I was thinking about providing something like:

{
  validation: ['<number>', '<integer>', '<angle>', 'auto']
}

But then there is an additional layer of indirection going on and we would have to parse those values and what not. So I don't think it's worth it really.

I kinda like that you have full control right now, and bail when using return null.

If anything, I think we could add some functions like isInteger, isNumber that abstracts the common Number.isInteger(Number(value)) related code.

Copy link
Member

@adamwathan adamwathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, I think missing handling the flex root though, because current alphas generate this for example:

.flex-direction {
  flex: direction;
}

@RobinMalfait RobinMalfait force-pushed the fix/validate-invalid-bare-utilities branch from d09ed4c to 73e2252 Compare March 14, 2024 17:48
@RobinMalfait
Copy link
Member Author

Looks good so far, I think missing handling the flex root though, because current alphas generate this for example:

.flex-direction {
  flex: direction;
}

Yep handled this one and also for stroke and fill utilities.

Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit about a comment but otherwise looks good!

RobinMalfait and others added 2 commits March 15, 2024 14:43
This way we make sure that the bare value ends with `%`, and the value
before it is a number.
@RobinMalfait RobinMalfait merged commit 154164d into next Mar 15, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the fix/validate-invalid-bare-utilities branch March 15, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants