github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/theme-data-revision/#/
Closed landondurnan closed 2 years ago
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/theme-data-revision/#/
github-actions[bot]
commented
2 years ago | File | Before | After |
|---|---|---|
components/ActionBar/styles.css |
5.5 kB |
-0.13%↓5.5 kB |
components/Button/styles.css |
3.6 kB |
-0.19%↓3.6 kB |
components/Calendar/styles.css |
2.1 kB |
-0.34%↓2.1 kB |
components/Choice/styles.css |
1.3 kB |
-0.53%↓1.3 kB |
components/Container/styles.css |
1.2 kB |
-1.92%↓1.2 kB |
components/PinInput/styles.css |
1.3 kB |
-0.53%↓1.3 kB |
components/SegmentedControl/styles.css |
1.0 kB |
-0.69%↓1.0 kB |
components/Stepper/styles.css |
476 B |
-1.47%↓469 B |
components/Textarea/styles.css |
1.5 kB |
-1.16%↓1.5 kB |
components/TextButton/styles.css |
1.5 kB |
-0.46%↓1.5 kB |
components/Theme/script.js |
7.3 kB |
-1.45%↓7.2 kB |
components/Theme/styles.css |
247 B |
-8.91%↓225 B |
| Total (Includes all files) | 1.2 MB |
-0.05%↓1.2 MB |
| Tarball size | 232.5 kB |
-0.01%↓232.5 kB |
🤖 This report was automatically generated by pkg-size-action
pretzelhammer
commented
2 years ago I've updated the PR title from refactor to refactor! because this contains breaking changes
pretzelhammer
commented
2 years ago It appears that setting the button patterns data in the default theme doesn't work as expected. In this PR, I'm setting default colors for the primary / secondary variants, but even testing these changes manually on the beta they aren't working.
That branch isn't working because the Buttons styleguide doesn't use patterns yet, it's still on variants. I've made a new branch which updates the Button styleguide to use patterns so it's now responsive to changes in the default theme.
pretzelhammer
commented
2 years ago Since we know that others are using what was previously considered internal CSS properties and that changes to those properties now constitute breaking changes -- we should consider prefixing CSS properties (and eventually classes) we're generating to prevent collisions. maker-[token] for example.
This is reasonable and should be its own PR. I've made a PR for it here: https://github.com/square/maker/pull/305
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/theme-data-revision/#/
pretzelhammer
commented
2 years ago Change: Our theme-default sets data for colors.elevation and colors.overlay however, the data we've been passing into the theme object has explicitly included a hyphenated version of the token, despite being part of the same colors object. We should change the theme to accept the same color tokens that are set in the theme-default and be consistent with how all the other colors are named.
I think this is probably an oversight or a bug. I think this is reasonable as its own standalone task so I've made a PR for it: https://github.com/square/maker/pull/307
pretzelhammer
commented
2 years ago Leave alone:
colors.textandcolors.headingas generic global tokens are intended to apply to all headings and text. Changing these names to names like paragraph and title insinuates that the colors apply to less of our output. The original names were not selected because of labels in our largest consuming app, but primarily to define a spec that is more inline with definitions in other systems and agnostic to ever-changing product opinions. Our design tokens and data shouldn't need to change due to a single product UI.
We have title and paragraph text patterns, and the colors apply to those patterns, so it makes sense that the title color applies to the title text pattern. I think it's reasonable to keep the beta names as they are. Also, our design and data shouldn't need to change to fall in line with some arbitrary 3rd-party open source spec.
Add:
colors.primaryandcolors.secondaryis a great addition that provides a reusable mapping to our components, but also subsequently custom components or styling based on globally defined color profiles. We discovered that one piece of data we're setting globally called outlineButtonColor is missing in the global data which should directly map to our secondary button variant. So setting / passing both primary and secondary colors would be helpful for buttons and likely future scenarios.
We have colors.primary, and as Lauren said we don't really need a colors.secondary. The convention at the moment is to use colors.paragraph for any secondary/outline buttons, both internally in Maker and within Website. If Website needs to customize this they can override it within the button secondary pattern.
Change: (Worth debating) Simplify the fonts object and split into fonts and fontWeights, makes our tokens simpler and easier to type check and automate processing in the future.
I disagree. I find this harder to write and harder to cognitively parse:
fonts: { // is really only font family data, not general font data
headline: 'etc',
title: 'etc',
paragraph: 'etc',
label: 'etc',
},
fontWeights: {
headline: '700',
title: '500',
paragraph: '400',
label: '500',
},
fontColors: {
headline: 'etc',
title: 'etc',
paragraph: 'etc',
label: 'etc',
}Than this:
fonts: {
headline: {
family: 'etc',
color: 'etc',
weight: '700',
},
title: {
family: 'etc',
color: 'etc',
weight: '500',
},
paragraph: {
family: 'etc',
color: 'etc',
weight: '400',
},
label: {
family: 'etc',
color: 'etc',
weight: '500',
},
},In the former example, if I want to understand how a headline will be rendered, I have to look at n-different sub-objects stitch it together in my head. In the latter example I just have to look at one place and all the headline data is there. The latter is much simpler imo.
- Change:
bodyinstead ofparagraph.- Change:
headinginstead oftitle.
Since we have title and paragraph text patterns I think it's nice to keep the global names consistent with the individual pattern names.
Change: CSS property naming conventions. As we defined with the colors we've begun to establish a good pattern of
[tokenCategory].[token].[value]and making our CSS properties consistent with those names. We've begun to establish the singular form of the token category. Socolors.primaryeasily translates to--color-primary. As we look at our fonts, we should do the same thing.fonts.bodyeasily translates to--font-body. Depending on some decisions make around the font vs fontWeight data structure -- this translation to the CSS property should be maintained.
No. We want to maintain a layer of abstraction between the theme spec & the public CSS API. We should never make any promises about how one is turned into the other. This is especially important if we ever want to make changes to the theme spec or the public CSS API without affecting the other. It's further important if we want to add design tokens to the theme spec that do not have a 1:1 relationship to some existing CSS property. A good example of this is shape. We may add shape: 'squared' | 'rounded' | 'pill' to the theme spec and it makes no sense to expose a --shape: 'squared' | 'rounded' | 'pill' to the public CSS API because that is not a valid CSS property.
I think exposing a
patternconcept as an alternative to both global design tokens andvariantsis premature, particularly since this concept is limited to two components where one is already using variants. While I appreciate it's solving a problem, I think we should keep the concept internal until we can refactor our code to have a more robust theming system withvariants.
patterns === variants. They're the same thing, just have a different name.
landondurnan
commented
2 years ago Much of this PR has been addressed in other smaller separate PRs.
Describe the problem this PR addresses
There's some changes in the v10 Beta theme configuration that make modifications to the theme data that are different from our plans and long-term goals.
Describe the changes in this PR
This PR updates the theme configuration data so that it's more compatible with existing data and theme data we plan to support in the future.
Other information
Details
The current theme data has a set of global design tokens primarily focused on Color, with v10 adding additional support for theming Typography. I've outlined the specific data changes in each version with more explanation on what we should add, change, or leave alone.
Colors
colors.textandcolors.headingas generic global tokens are intended to apply to all headings and text. Changing these names to names likeparagraphandtitleinsinuates that the colors apply to less of our output. The original names were not selected because of labels in our largest consuming app, but primarily to define a spec that is more inline with definitions in other systems and agnostic to ever-changing product opinions. Our design tokens and data shouldn't need to change due to a single product UI.colors.primaryandcolors.secondaryis a great addition that provides a reusable mapping to our components, but also subsequently custom components or styling based on globally defined color profiles. We discovered that one piece of data we're setting globally calledoutlineButtonColoris missing in the global data which should directly map to our secondary button variant. So setting / passing both primary and secondary colors would be helpful for buttons and likely future scenarios.colors.elevationandcolors.overlayhowever, the data we've been passing into the theme object has explicitly included a hyphenated version of the token, despite being part of the same colors object. We should change the theme to accept the same color tokens that are set in the theme-default and be consistent with how all the other colors are named.Fonts (and fontWeights)
fontsandfontWeights, makes our tokens simpler and easier to type check and automate processing in the future. This data format is also compatible with the Theme UI spec which is becoming more ubiquitous in more systems. But the primary benefit is that this format is easier to type and remember:fonts.bodyis simpler thanfonts.paragraph.fontFamilybodyinstead ofparagraph. The default font family and weight is intended to apply to everything in the theme, not just paragraphs. Defining this as theparagraphincreases confusion as where this style will apply, wherebodyis a far more common way of defining this default as seen in numerous other design systems and type tools [1][2][3][4][5][6]headinginstead oftitle. Over the years we've found ourselves coming up with different terms for essentially the same thing. From an editorial perspective there's subheading, subtitle, headline, headers, titles, etc. But from a semantic HTML perspective, these are all just different labels for essentially setting a h1-h6 (i.e. Heading) tags in a document. Heading is more generic and ubiquitous as an all encompassing term. It's also a far more common way of defining this token in numerous other design systems [1][2][3][4] It's worth noting that systems like Material / Apple when providing multiple typographic styles for "headings" they have made distinctions ofDisplay || Headline || Title, so perhaps this is worth some debate. But I still think looking at the full landscape, thatheadingis more common (and why we had a Heading component, instead of a Title component, to begin with)colorswe've begun to establish a good pattern of[tokenCategory].[token].[value]and making our CSS properties consistent with those names. We've begun to establish the singular form of the token category. Socolors.primaryeasily translates to--color-primary. As we look at our fonts, we should do the same thing.fonts.bodyeasily translates to--font-body. Depending on some decisions make around the font vs fontWeight data structure -- this translation to the CSS property should be maintained. In the short-term for consistency, but in the long-term so we could automatically generate these global tokens into CSS more easily. It's worth some additional discussion about how we feel about a mix of pascal + hyphens in CSS properties. A lot of systems leveraging tools like styled components, emotion, stitches all translate design tokens wholesale where pascal token names likelineHeightsin the CSS properties. I'm not as strongly opinionated about this since we can currently manually translate these object keys to hyphen and later automate that, but it's likely more common in other systems to just use hyphens.Additional considerations and feedback from testing beta + labs
betawith a nested theme. I was attempting to test theme reactivity and mimic our internalThemeBlockcomponent. I'm not sure if I have it setup correctly, but it's not reactive to changes to the parent theme. (I'll push up a lab deploy that shows the issue in isolation outside of this PR)inheritmakes things work a bit differently than expected. While the text component resolves the theme data and inlines the font family, that's not happening everywhere -- even with the theme setting a default font. I figured we should be a bit more opinionated and set a default tosystem-ui, it might be masking the issue, but it appears to work better. (I'll push up a lab deploy that also shows this)patternconcept as an alternative to both global design tokens andvariantsis premature, particularly since this concept is limited to two components where one is already using variants. While I appreciate it's solving a problem, I think we should keep the concept internal until we can refactor our code to have a more robust theming system with variants.variantsare by far the most common way of documenting how to get alternative styles on a component in every system. We can add variants to the text component the same way we are doing it for buttons. The public API can remain consistent and meets expectations for the prop to allow alternative styling options.maker-[token]for example.