Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add missing unitless CSS properties #19286

Closed
wants to merge 1 commit into from
Closed

Add missing unitless CSS properties #19286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/react-dom/src/shared/CSSProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
* CSS properties which accept numbers but are not in units of "px".
*/
export const isUnitlessNumber = {
animation: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This one looks wrong to me. I assume it refers to the "duration" or "delay" parts of the shorthand? Looking at the animation-duration docs, it says:

The time that an animation takes to complete one cycle. This may be specified in either seconds (s) or milliseconds (ms). The value must be positive or zero and the unit is required. A value of 0s, which is the default value, indicates that no animation should occur.

The animation-delay docs say the same:

The time offset, from the moment at which the animation is applied to the element, at which the animation should begin. This may be specified in either seconds (s) or milliseconds (ms). The unit is required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit hidden under the grammar for animation: <time> || <easing-function> || <time> || <single-animation-iteration-count> || <single-animation-direction> || <single-animation-fill-mode> || <single-animation-play-state> || [ none | <keyframes-name> ] where <single-animation-iteration-count> is valid syntax which can be a single number.

Reproducible in chrome:
animation-shorthand-unitless

--https://codesandbox.io/s/animation-unitless-mg06t

Copy link
Contributor

@bvaughn bvaughn Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Thank you for clarifying.

I'm still concerned that changing animation may be a bad idea, since the majority of the numeric values this shorthand property covers do require units.

Then again, as I've mentioned elsewhere on this PR, this is not my area of expertise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since animation: 1px is meaningless/invalid, I don’t think adding animation can break anything, at least (except if perhaps someone has a unitless number that gets px and thus ignored today).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not familiar with this part of the code, so I don't know what the implications of adding this rule for a shorthand property that can contain multiple numeric values. I agree animation: 1px is meaningless though 😆

animationIterationCount: true,
borderImage: true,
borderImageOutset: true,
borderImageSlice: true,
borderImageWidth: true,
Expand All @@ -24,6 +26,7 @@ export const isUnitlessNumber = {
flexShrink: true,
flexNegative: true,
flexOrder: true,
fontSizeAdjust: true,
gridArea: true,
gridRow: true,
gridRowEnd: true,
Expand All @@ -34,11 +37,19 @@ export const isUnitlessNumber = {
gridColumnSpan: true,
gridColumnStart: true,
fontWeight: true,
initialLetter: true,
lineClamp: true,
lineHeight: true,
maskBorder: true,
maskBorderOutset: true,
maskBorderSlice: true,
maskBorderWidth: true,
maxLines: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This property isn't standardize and I think has been superseded by line-clamp? So maybe we shouldn't add it here?

Copy link
Author

Choose a reason for hiding this comment

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

That’s right, I just wanted to add any property with a status of standard or experimental.

opacity: true,
order: true,
orphans: true,
scale: true,
shapeImageThreshold: true,
tabSize: true,
widows: true,
zIndex: true,
Expand Down