-
Notifications
You must be signed in to change notification settings - Fork 66
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
[BD-46] feat: add description for tokens #2112
[BD-46] feat: add description for tokens #2112
Conversation
Thanks for the pull request, @monteri! When this pull request is ready, tag your edX technical lead. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## alpha #2112 +/- ##
=======================================
Coverage 90.77% 90.77%
=======================================
Files 213 213
Lines 3501 3501
Branches 843 843
=======================================
Hits 3178 3178
Misses 315 315
Partials 8 8 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I really like the idea of having descriptions for design tokens. I think there's a lot of potential here.
This PR raises quite a few questions around how we want to use those descriptions. I left some comments with the ones that came to mind.
Hopefully these comments can spark some discussion!
tokens/src/core/alias/size.json
Outdated
"base": { "value": "6px", "type": "dimension", "source": "$border-radius" }, | ||
"lg": { "value": "7px", "type": "dimension", "source": "$border-radius-lg" }, | ||
"sm": { "value": "4px", "type": "dimension", "source": "$border-radius-sm" } | ||
"base": { "value": "6px", "type": "dimension", "source": "$border-radius", "description": "Border radius basic." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use the term "basic" for this. One alternative that comes to mind is "default." I'd like to hear some thoughts on this before settling on a naming convention for these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree, default
sounds better, or maybe even just base
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to default
here
tokens/src/core/alias/size.json
Outdated
@@ -1,11 +1,11 @@ | |||
{ | |||
"size": { | |||
"border": { | |||
"width": { "value": "1px", "type": "dimension", "source": "$border-width" }, | |||
"width": { "value": "1px", "type": "dimension", "source": "$border-width", "description": "Border width value." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to have every token include a description? I don't think "Border width value" provides any any additional context over border-width
or side.border.width
.
If the idea is to be able to rely exclusively on descriptions then I understand having this, but I would suggest simply "Border width" without "value"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually we want to have description for every token, but not in this PR. At some point in the future we will have a UI to update token values without explicitly opening a PR (e.g., designers could update token values without engineers' help), and ideally we'd also want to have some kind of a description for tokens in that UI, our idea is to store and take such descriptions from the JSON files.
I agree though that most of the token names are self-explanatory and maybe we don't need descriptions for every token, on the other hand we could add it just to be consistent.
Also, yeah, value
is redundant in the description, I'd suggest Default border with
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Default border width
tokens/src/core/alias/size.json
Outdated
"lg": { "value": "7px", "type": "dimension", "source": "$border-radius-lg", "description": "Border radius large." }, | ||
"sm": { "value": "4px", "type": "dimension", "source": "$border-radius-sm", "description": "Border radius small." } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another place to decide on naming conventions. My gut feeling is "Large border radius" feels better than "Border radius large.", but I'd like to hear more thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the same way 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Large border radius
and for the second one also
"xs": { "value": "0", "type": "dimension", "description": "Starting value for extra-small breakpoint." }, | ||
"sm": { "value": "576px", "type": "dimension", "description": "Starting value for small breakpoint." }, | ||
"md": { "value": "768px", "type": "dimension", "description": "Starting value for medium breakpoint." }, | ||
"lg": { "value": "992px", "type": "dimension", "description": "Starting value for large breakpoint." }, | ||
"xl": { "value": "1200px", "type": "dimension", "description": "Starting value for extra-large breakpoint." }, | ||
"xxl": { "value": "1400px", "type": "dimension", "description": "Starting value for extra-extra-large breakpoint." } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something special about these that warrants adding "Starting value" to the description? I could also see adding more context via these descriptions such as why each breakpoint was picked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, we should also describe intention of the breakpoints, e.g. for which device they are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Starting breakpoint for portrait phones
and so on for other breakpoints
tokens/src/core/global/display.json
Outdated
"base": { "value": "1", "type": "dimension", "source": "$display-line-height", "description": "Line height basic value." }, | ||
"mobile": { "value": "3.5rem", "type": "dimension", "source": "$display-mobile-line-height", "description": "Line height for mobile devices." } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like "Standard line height" and "Mobile line height" instead here? To reiterate, don't take these comments as prescriptive, I'm just trying to spark conversation around what our description conventions should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you variant is more human-friendly, so I would use it instead 🙂 Also, I think we definitely should avoid using word value
in the description, it just doesn't contain any useful information for the reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "Standard line height" and "Mobile line height"
tokens/src/core/global/display.json
Outdated
"1": { "value": "3.75rem", "type": "dimension", "source": "$display1-size", "description": "Font size of level 1." }, | ||
"2": { "value": "4.875rem", "type": "dimension", "source": "$display2-size", "description": "Font size of level 2." }, | ||
"3": { "value": "5.625rem", "type": "dimension", "source": "$display3-size", "description": "Font size of level 3." }, | ||
"4": { "value": "7.5rem", "type": "dimension", "source": "$display4-size", "description": "Font size of level 4." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"level 1" etc. doesn't seem to add any context here. Not sure what we should have here instead though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here would be a good idea to look where we use these variables and come up with more detailed description. I think these are font sizes for headers? Maybe the description would be something like Base font size for h1 element.
(not ideal but probably better than saying level 1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "Font size for heading of level 1." and so on
"0": { "value": 0, "type": "ratio", "description": "z-index of level 0." }, | ||
"200": { "value": 200, "type": "ratio", "description": "z-index of level 200." }, | ||
"400": { "value": 400, "type": "ratio", "description": "z-index of level 400." }, | ||
"600": { "value": 600, "type": "ratio", "description": "z-index of level 600." }, | ||
"800": { "value": 800, "type": "ratio", "description": "z-index of level 800." }, | ||
"1000": { "value": 1000, "type": "ratio", "description": "z-index of level 1000." }, | ||
"1200": { "value": 1200, "type": "ratio", "description": "z-index of level 1200." }, | ||
"1400": { "value": 1400, "type": "ratio", "description": "z-index of level 1400." }, | ||
"1600": { "value": 1600, "type": "ratio", "description": "z-index of level 1600." }, | ||
"1800": { "value": 1800, "type": "ratio", "description": "z-index of level 1800." }, | ||
"2000": { "value": 2000, "type": "ratio", "description": "z-index of level 2000." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These feel like another example of the "do we expect to only see the descriptions?" question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good example of redundant description if we are able to see variable name, which we most likely will.
I can't come up with any meaningful description here, maybe we should just leave it empty.
@adamstankiewicz curious what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave as is for the time being, but plan on revisiting once we have more clarity how these descriptions will ultimately get used. Goal of this PR is largely to get some reasonable descriptions in place to solidify the decision to do so and showcase a few examples.
"base": { "value": "all .2s ease-in-out", "type": "transition", "source": "$transition-base", "description": "Generic transition for any property change" }, | ||
"fade": { "value": "opacity .15s linear", "type": "transition", "source": "$transition-fade", "description": "Opacity transition that takes 150ms" }, | ||
"collapse": { "value": "height .35s ease", "type": "transition", "source": "$transition-collapse", "description": "Collapse transition that takes 350ms" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These raise the question of what is being described. Is the point of the description to describe the value or the variable?
"Generic transition for any property changed" seems to describe the variable, since it has no mention of "ease-in-out" or 200ms.
The other two seem to describe the value, directly mentioning the type of transition and amount of time.
I'd think the goal would be to describe the variable, so when someone sets a new value for it they'd know where it would be used. This would prevent the situation where someone updates the fade transition to be slower and the description still says "150ms"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with that, we should only describe the variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left here because there can be more transitions and description should specify the difference between them so I assume that timings will be user-friendly here. Yes, they shouldn't be forgotten when those values will be changed in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable for now to include the value in this case. Can always revisit later. Generally, though, I agree the descriptions should be describing the intent of the variable, not so much the value itself.
The descriptions will be mostly useful to theme authors to get a better idea of what each design token's purpose is.
} | ||
}, | ||
"serif": { "value": "serif", "source": "$font-family-serif" }, | ||
"serif": { "value": "serif", "source": "$font-family-serif", "description": "Serif font style" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other ones say "font family" but this one stays "font style"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Serif font family.
@brian-smith-tcril thanks for the great feedback! That's much appreciated 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as a first pass of sample descriptions. We can revisit descriptions again once there's more clarity around how they will be used/written.
"0": { "value": 0, "type": "ratio", "description": "z-index of level 0." }, | ||
"200": { "value": 200, "type": "ratio", "description": "z-index of level 200." }, | ||
"400": { "value": 400, "type": "ratio", "description": "z-index of level 400." }, | ||
"600": { "value": 600, "type": "ratio", "description": "z-index of level 600." }, | ||
"800": { "value": 800, "type": "ratio", "description": "z-index of level 800." }, | ||
"1000": { "value": 1000, "type": "ratio", "description": "z-index of level 1000." }, | ||
"1200": { "value": 1200, "type": "ratio", "description": "z-index of level 1200." }, | ||
"1400": { "value": 1400, "type": "ratio", "description": "z-index of level 1400." }, | ||
"1600": { "value": 1600, "type": "ratio", "description": "z-index of level 1600." }, | ||
"1800": { "value": 1800, "type": "ratio", "description": "z-index of level 1800." }, | ||
"2000": { "value": 2000, "type": "ratio", "description": "z-index of level 2000." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave as is for the time being, but plan on revisiting once we have more clarity how these descriptions will ultimately get used. Goal of this PR is largely to get some reasonable descriptions in place to solidify the decision to do so and showcase a few examples.
"base": { "value": "all .2s ease-in-out", "type": "transition", "source": "$transition-base", "description": "Generic transition for any property change" }, | ||
"fade": { "value": "opacity .15s linear", "type": "transition", "source": "$transition-fade", "description": "Opacity transition that takes 150ms" }, | ||
"collapse": { "value": "height .35s ease", "type": "transition", "source": "$transition-collapse", "description": "Collapse transition that takes 350ms" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable for now to include the value in this case. Can always revisit later. Generally, though, I agree the descriptions should be describing the intent of the variable, not so much the value itself.
The descriptions will be mostly useful to theme authors to get a better idea of what each design token's purpose is.
@monteri 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 21.0.0-alpha.17 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 22.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add description for tokens.
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist