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

Adds numeric, string, position, and url CSS types #2111

Closed
wants to merge 8 commits into from

Conversation

vwallen
Copy link
Contributor

@vwallen vwallen commented Oct 29, 2024

Adding a number of fundamental and domain specific CSS types. Also creates a types group under css.

A few include -type as part of the name to disambiguate from similarly named objects.

The top level dimension type is included with numeric types, but it could also reasonably be its own feature (with or with out including sub-types such as angel). As included here, it's included with other numeric types and each sub-type has its own feature.

@github-actions github-actions bot added the feature definition Creating or defining new features or groups of features. label Oct 29, 2024
features/url-type.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

This feature set looks like a good way of communicating these keys, but I think I'd like at least one other opinion on that.

Co-authored-by: James Stuckey Weber <[email protected]>
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

OK, so I'm a little bit worried about CSS types. My concern is that a type, by itself, doesn't really do anything. Is an abstract type, that has no API of its own, a feature? I'm inclined to say no.

We have a few CSS types (e.g., conic gradients) already but, as far as I know, they are pretty closely circumscribed, at least with the description. "The conic-gradient() and repeating-conic-gradient() CSS functions create backgrounds…" They're "types" yes, but implicitly they're types for the background property etc.

This implies to me that some of these types, such as they are, ought to live with actual properties. Some of this is an upstream problem of BCD (i.e., we ought to de-mixin types and situate type support with specific properties), but we're stuck with confronting it here. And ideally we'd be able to put keys into multiple features. But without the option to do that, I think it makes sense to put them with the oldest properties they correspond to (if they're foundational) or reference them (if they're distinct later additions).

That said, this is a worry, not an edict. I'm also no CSS expert. I'm going to mark this up with some thoughts, but I'm not strongly holding them. Please tell me if I've missed any marks.

Comment on lines +6 to +11
- css.types.angle
- css.types.angle-percentage
- css.types.angle.deg
- css.types.angle.grad
- css.types.angle.rad
- css.types.angle.turn
Copy link
Collaborator

Choose a reason for hiding this comment

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

My hunch here is that these belong with linear-gradient() (being the younger of it and transform).

@@ -0,0 +1,6 @@
name: calc-size
description: The `calc-size()` CSS function computes mathematical expressions that include keyword values such as `calc-size(fit-content, size / 2)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know a lot about this one, but I think it makes sense as a stand alone feature, being a pretty distinct later addition to height and width. Can we explicitly mention height and width here? I'm imagining something that reads like…

The calc-size() CSS function computes a height and width value based on [mumbles]…

I suspect we also need a "Not to be confused with the calc() function."

@@ -1,7 +1,12 @@
name: calc()
description: The `calc()` CSS function computes mathematical expressions such a `calc(100%/3 - 1em)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an exception that proves the rule, by the way. 😅

@@ -0,0 +1,9 @@
name: frequency
description: The `frequency` CSS type represents a frequency value, expressed in hertz (`Hz`), or kilohertz (`kHz`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

MDN says, "It is not currently used in any CSS properties." Is that true?

If so, I think we should drop this feature and push to remove them from BCD.

Comment on lines +6 to +7
- css.types.length
- css.types.length-percentage
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should put these with the width-height feature, or something else sufficiently ancient (borders?). This is too generic to be meaningfully recognizable as a "feature" that a developer might even think about as using.

Comment on lines +6 to +10
- css.types.dimension
- css.types.integer
- css.types.number
- css.types.number.scientific_notation
- css.types.percentage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I'm struggling to think of these as a feature of CSS. Can we put them somewhere ancient and plausible? Perhaps…

  • dimension → height-width
  • integer → z-index
  • number → opacity
  • percentage → margin

Comment on lines +6 to +8
- css.types.position
- css.types.position.four_value_syntax
- css.types.position.keyword_value_syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move these to where background-position ends up?

Comment on lines +6 to +7
- css.types.string
- css.types.string.unicode_escaped_characters
Copy link
Collaborator

Choose a reason for hiding this comment

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

The content property?

Comment on lines +6 to +7
- css.types.time
- css.types.time-percentage
Copy link
Collaborator

Choose a reason for hiding this comment

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

To animations-css?

spec: https://drafts.csswg.org/css-values-4/#urls
group: types
compat_features:
- css.types.url
Copy link
Collaborator

Choose a reason for hiding this comment

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

background-image?

@vwallen vwallen marked this pull request as draft November 5, 2024 15:46
@vwallen
Copy link
Contributor Author

vwallen commented Nov 5, 2024

Converting this to draft to retain the conversation. Will take up these changes and split up into appropriate new PRs

@jamesnw
Copy link
Collaborator

jamesnw commented Nov 7, 2024

The split out PRs have been merged 🎉

@jamesnw jamesnw closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature definition Creating or defining new features or groups of features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants