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

Update composite types chapter to reflect decision made on issue #63 #86

Merged
merged 17 commits into from
Feb 3, 2022

Conversation

c1rrus
Copy link
Member

@c1rrus c1rrus commented Dec 5, 2021

Closes #54

The format editors have decided to keep the concept of composite types in the draft spec. However, we will replace the mechanism for user-defined composite types with a set of pre-defined composite types. (Future spec versions may bring back user-defined (composite) types if there is demand for it).

@netlify
Copy link

netlify bot commented Dec 5, 2021

✔️ Deploy Preview for dtcg-tr ready!

🔨 Explore the source changes: 2ca41f2

🔍 Inspect the deploy log: https://app.netlify.com/sites/dtcg-tr/deploys/61fabf72da788500073346ba

😎 Browse the preview: https://deploy-preview-86--dtcg-tr.netlify.app

@kevinmpowell
Copy link
Contributor

kevinmpowell commented Dec 13, 2021

@c1rrus I'm not convinced we need the GradientStop type.
I suggest Gradient's type definition be: "an array of objects with Color and Number values"

What value does the GradientStop provide?

@c1rrus
Copy link
Member Author

c1rrus commented Dec 13, 2021

@kevinmpowell Yeah, I wasn't too keen on adding the gradient-stop type myself.

My reasoning for doing it though was as follows: In the composite type intro, we say that composite type values are either objects whose properties are sub-values, or they are an array of sub-values. Sub-values in turn are defined as a value from one of the types or a reference to a token of the same type.

Therefore, having an array of objects which themselves are not sub-values, but whose properties are doesn't quite fit that definition. Breaking it up into 2 types gets around that.

But... it's clunky.

So, I'll have a go at rewording our definition of composite types so that it's broader. Then I can get rid of gradient-stop as its own type.

@c1rrus
Copy link
Member Author

c1rrus commented Dec 13, 2021

@kevinmpowell Ok, just pushed an update that merges the gradient stop stuff into the gradient type.

- `fontSize`: The size of the typography. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `fontWeight`: The weight of the typography. The value of this property must be a valid JSON string or a reference to a string token.
- `letterSpacing`: The horizontal spacing between characters. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `lineHeight`: The vertical spacing between lines of typography. The value of this property must be a valid JSON string or a reference to a string token.
Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't a number or dimension (or both) be more appropriate here?

Otherwise, if it's a string value, then what's the expected syntax within that string value?

Copy link
Contributor

Choose a reason for hiding this comment

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

CSS often uses unitless line heights, which would push me toward "number" for the type, but that would exclude the definition of a line height that includes the unit, hence "string"

Our current dimension Type requires either px or rem excluding the possibility of using unitless line heights,

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not both? Could we not say the lineHeight sub-value must be a valid dimension or number value?

Perhaps the ability for sub-types to be one of several types could be (yet) another special quality of composite types?

Copy link
Member

Choose a reason for hiding this comment

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

lineHeight could be a type itself, which is a union type of: number, dimension, percentage, or keyword 'normal' (based on the CSS spec at least: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should allow users to define unitless line heights. Hence, I agree with @dbanksdesign and I think we could add a lineHeight type.

Then, this example...
- `fontName`: The typography's font. The value of this property must be a valid [font name](#font-name) or a reference to a font name token.
- `fontSize`: The size of the typography. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `fontWeight`: The weight of the typography. The value of this property must be a valid JSON string or a reference to a string token.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we restrict this to a pre-defined set of values (effectively, a sort of enum). E.g. "bold", "normal", "light", etc. and/or numeric values like 100, 200, 300, etc.?

Otherwise, any string is technically valid, so how should a tool interpret something like "fontWeight": "hello"?

If we do limit the values, I think it might make sense to define that as its own type (maybe "font-weight" or something like that)? It's conceivably something people might also define individual tokens for.

Copy link
Contributor

@drwpow drwpow Dec 14, 2021

Choose a reason for hiding this comment

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

Variable fonts usually have weight be a number from 1–999, so it’d be nice if numbers could be allowed as @c1rrus said.

Should we restrict this to a pre-defined set of values (effectively, a sort of enum)

This might get tricky. “bold” is almost a standard, but other typefaces differ. For example, 400 is either called “regular” or “book” depending on the typeface. And below 400, you get different qualifiers like “thin”, “ultrathin”, “light”, “ultralight,” etc. There are probably words that apply to most fonts, but users may find it confusing if the fonts they use disagree on terminology.

(Also hello! 👋🏻 I’ve been following this repo for a while and I’d like to be helpful and join in the discussions, but please disregard if this is disruptive or off-base)

Copy link
Contributor

Choose a reason for hiding this comment

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

@drwpow Thanks a lot for sharing your thoughts on this. The more feedback we get from the community the better it is tbh.

I agree with @drwpow regarding the fontWeight property. It's a tricky one because there's no clear convention.

In my opinion its value should be restricted to a set of string and numeric values.

String values:

  • ultrathin
  • thin
  • ultralight
  • light
  • regular
  • book
  • normal
  • bold
  • ultrabold

I might have forgotten other frequently used values here though. The tricky part is that there's no strict convention between typefaces for these values.

Numeric values from 1 to 999.

Copy link
Member Author

Choose a reason for hiding this comment

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

Inspired by @ChucKN0risK & @drwpow's comments above, I propose the following:

  • We add a new (non-composite) fontWeight type
  • The fontWeight sub-value of the typography composite type must have values that are valid fontWeight token values or references to fontWeight tokens.

The new fontWeight type's values must be either a number value in the range 1 - 1000 (which seems to be the range CSS and Android permit) or one of the pre-defined strings listed in the table below. As per the table, the string names are just human-friendly aliases for certain number values, just like how CSS does it, which in turn seems to be based on the OpenType spec:

numeric value string value aliases
100 thin, hairline
200 extraLight, ultraLight
300 light
400 normal, regular, book
500 medium
600 semiBold, demiBold
700 bold
800 extraBold, ultraBold
900 black, heavy
950 extraBlack, ultraBlack

Sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

I support this proposal @c1rrus

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I've just pushed a commit that adds it and updates the typography type accordingly.

I went for a kebab case font-weight name rather than camel case as I used in my comment above in order to be consistent with the cubic-bezier type we already have. I also renamed the existing font type to font-name to make its purpose less ambiguous.

I wonder whether we should also change the sub-value names we've used in composite types to be kebab-case for the sake of consistency (or else, make everything camel case). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we should also change the sub-value names we've used in composite types to be kebab-case for the sake of consistency (or else, make everything camel case). What do you think?
@c1rrus You're suggesting to rename extraLight as extra-light?
If so, I agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ChucKN0risK not quite. I already used kebab case for the values (e.g. extra-light). But the names of the typography type's sub-values are currently camel case:

{
  "example token": {
    "type": "typography",
    "value": {
      "fontWeight": "extra-light" // <-- this looks weird to me

      // ...
    }
  }
}

So, what I am proposing is that we make those kebab-case too: "font-weight": "extra-light".

Furthermore, we should then adopt this convention across our entire format.

Choose a reason for hiding this comment

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

+1 to kebab-case everywhere

Just as with “normal” types for tokens, using a custom, composite type will allow tools to check that the values you use match the expected type. In our color pair example above, attempting to do the following would be invalid and tools should ignore the token and show an error to the user, since “Comic Sans MS” is not a valid color value.
- `color`: The color of the border. The value of this property must be a valid [color value](#color) or a reference to a color token.
- `width`: The width or thickness of the border. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `style`: The border's style, for example "solid" or "dashed". The value of this property must be a valid JSON string or a reference to a string token.
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to font weight on the typography type, I wonder whether there should be a stand-alone border-style type that defines a set of known, permitted values (e.g. "solid", "dashed", "dotted", etc.).

Otherwise any string would be valid and its unclear how tools should interpret the value

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Its value should be restricted to a set of the following strings: solid, dashed, dotted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... on second thoughts, a borderStyle (or perhaps strokeStyle is a better name?) type might need to be more complex. I've done a little bit of research and there's a few ways different platforms and tools do this:

  • CSS has a finite number of "line style" keywords (solid, dashed, dotted, etc.), but doesn't offer finer control beyond that.
  • SVG has an attribute called stoke-dasharray, which lets you define a patterns of dash and gap lengths. There's also a stroke-linecap for round / butt / square caps.
  • Figma has "Solid" / "Dash" / "Custom" styles. For dash and custom, you can set the dash & gap lengths in pretty much the same way as SVG. It also lets you choose from the same 3 linecap styles that SVG has, except they call it "Dash cap".
  • Android seems to let you set dash width and dash gap (so equivalent to Figma's "Dash" style), but doesn't support custom patterns like SVG & Figma do

This is of course not an exhaustive list (if anyone knows what iOS, Windows or other things support, then please share), but I think it's safe to say that a stroke style type is non-trivial.

Perhaps strokeStyle is itself a composite type? For example, we might define its values as objects with the following properties:

  • dashArray: An array of dimension values, which could also be references to dimension tokens (as SVG does it)
  • lineCap: One of the following strings "round", "butt", "square"

For example:

{
  "line-dashed": {
    "type": "strokeStyle",
    "value": {
      "dashArray": ["3px", "{size-medium}"],
      "lineCap": "round"
    }
  },
  
  "size-medium": {
    "type": "dimension",
    "value": "5px"
  },
  
  "button-border": {
    "type": "border",
    "value": {
      "color": "#ff0000",
      "width": "{size-medium}",
      "style": "{line-dashed}"
    }
  }
}

But, similar to what I suggested for fontWeight in an earlier PR comment, we may also want to have a set of keywords (similar to CSS) for common stroke styles. In that case, we ought to define some kind of mapping between the two. For example we might define "dashed" to be an alias for { "dashArray": ["3px", "3px"], "lineCap": "butt" }. In that case, the following would also be valid:

{
  "line-dashed": {
    "type": "strokeStyle",
    "value": "dashed"
  }
}  

One catch here is that the CSS line styles seem to scale relative to the border's width. So, the dashes and gaps on a 10px border are wider than they are on a 1px border. As far as I can tell, in SVG and Figma this is not the case - the dash and gap values are essentially absolute lengths that remain the same regardless of how wide the border is. I threw together a little CodePen to illustrate this. At least in SVG, I couldn't figure out a way to make a dasharray scale relative to the stroke width. In other words SVG/Figma can do stuff CSS can't and CSS and do stuff that SVG/Figma can't! I have no idea what Android or iOS do in this respect (maybe @dbanksdesign knows?).

So maybe a mapping table isn't feasible?


On a separate note, going down this route would also be setting an interesting precedent, since we'd have a token type that is both a composite and non-composite type in one! I can think of other cases where that might be useful. Do we need a name for that? "Hybrid type" perhaps? ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@c1rrus thanks for all the research you did on this. I think a strokeStyle composite type is needed, but should have the shorthand you're describing to align with CSS behaviors.

I don't think we need a separate name for this type of token. It's still a composite token, it just has a shorthand for setting some of its sub values.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinmpowell I've just pushed a commit that adds a new stroke-style composite type and updates the border type to use that.

@c1rrus
Copy link
Member Author

c1rrus commented Jan 3, 2022

+@kevinmpowell

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Looking good! Added a few comments

### Fallbacks
- `color`: The color of the shadow. The value of this property must be a valid [color value](#color) or a reference to a color token.
- `x`: The horizontal offset that shadow has from the element it is applied to. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `y`: The vertical offset that shadow has from the element it is applied to. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
Copy link
Member

Choose a reason for hiding this comment

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

Although this might be more verbose, should we call x and y offsetX and offsetY respectively? The CSS spec and iOS use the term offset:

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

- `fontSize`: The size of the typography. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `fontWeight`: The weight of the typography. The value of this property must be a valid JSON string or a reference to a string token.
- `letterSpacing`: The horizontal spacing between characters. The value of this property must be a valid [dimension value](#dimension) or a reference to a dimension token.
- `lineHeight`: The vertical spacing between lines of typography. The value of this property must be a valid JSON string or a reference to a string token.
Copy link
Member

Choose a reason for hiding this comment

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

lineHeight could be a type itself, which is a union type of: number, dimension, percentage, or keyword 'normal' (based on the CSS spec at least: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height)


If so, which composites should be included initially?
Every shadow style has the exact same parts (color, X & Y offsets, etc.), but their respective values will differ. Furthermore, each part's value (which is also known as a "sub-value") is always of the same type. A shadow's color must always be a [color](#color) value, its X offset must always be a [dimension](#dimension) value, and so on. Shadow styles are therefore combinations of values _that follow a pre-defined structure_. In other words, shadow styles are themselves a type. We call types like this **composite types**.
Copy link
Member

Choose a reason for hiding this comment

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

Minor: should we not use "we" in the spec? "We call types like this..." v. "Types like this are called..."

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we shouldn't use "we" in the spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I'll fix that.

@c1rrus
Copy link
Member Author

c1rrus commented Jan 13, 2022

At the last format editors' call we decided to merge this PR more or less as-is and then open issues for each of the new composite types, so that we could have more focussed discussions for each and, where needed, open separate PRs to refine them.

I have therefore opened a bunch of new GitHub issues to collect feedback and pushed a commit that:

  • adds an initial stab at the missing transition type (so that there's something for folks to critique rather than it being completely open)
  • adds links in each composite type section to the respective GitHub issue

I suggest we do a final review of this PR (focussing more on things like grammar, typos, etc. rather than the content itself) so that we can get this merged. Discussion and work on the various composite types can then continue in the new issues.

@c1rrus c1rrus marked this pull request as ready for review January 13, 2022 21:41
@c1rrus c1rrus changed the title WIP: Update composite types chapter to reflect decision made on issue #63 Update composite types chapter to reflect decision made on issue #63 Jan 13, 2022

Represents a typographic style. The type property must be set to the string “typography”. The value must be an object with the following properties:

- `fontName`: The typography's font. The value of this property must be a valid [font name](#font-name) or a reference to a font name token.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the interest of sticking to kebab-case, what about naming this font-family? Otherwise it’d be the only property in typography that doesn’t map 1:1 to CSS.

Also, being pedantic, it technically is referring to a family because the name would include weight + italic/regular

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, thanks! @mkeftz made the same observation in #102.

I'll change it to fontFamily (at the last editor's call we decided to standardise on camel case, so I might as well change. it accordingly now).

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! This is probably one of the biggest PRs we've had to date and represents some amazing work. Thank you @c1rrus for driving this!

thank you

@c1rrus c1rrus merged commit e667526 into main Feb 3, 2022
@c1rrus c1rrus deleted the composite-types branch February 3, 2022 21:59
github-actions bot added a commit that referenced this pull request Feb 3, 2022
…86)

SHA: e667526
Reason: push, by @c1rrus

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Feb 3, 2022
…86)

SHA: e667526
Reason: push, by @c1rrus

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Should composites be part of the MVP specification?
6 participants