feat: add support for !default in SCSS variables format #359

jmmcduffie · 2019-11-23T20:19:51Z

Issue #, if available: #307

Description of changes:
This addresses the feature request made in #307 in one way. For my use case I'd like to make some variables "themeable" with !default but not all. For example, base colors should be immutable but the default text color could be configurable by the consumer of the design tokens.

I chose the key "themeable" because of seeing that language in the design systems community (for example with the Lightning Design System or from Brad Frost). And that way other formats could implement their own version of what theme-ability is.

This doesn't address setting !default on all variables through the configuration, but it should be easy enough to add that to what I've implemented.

Thanks in advance for considering this PR!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For any token definition that includes the property `themeable: true`, a `!default` will now be added in the scss/variables format. GitHub issue amzn#307

dbanksdesign

Thank you for this PR! I really like the idea. I would like to hear what others think about themeable as the property name. It makes sense to me, but would like to hear other opinions and to also think about how this might be used in other platforms like Android and iOS.

dbanksdesign · 2019-12-04T19:20:48Z

lib/common/formats.js

+      to_ret_prop += (prop.attributes.category==='asset' ? '"'+prop.value+'"' : prop.value);
+
+      if (prop.themeable === true) {
+        to_ret_prop += ' !default';


This function, variablesWithPrefix, is used to generate less and CSS variables as well. Would this addition break those formats? We might need to rethink this function...

Great catch. I assumed it was just shared between the different Sass formatters so that's my bad.

It would definitely break both of those other formats without some more work. I could add an additional argument to this function to output Sass-specific properties like !default.

I took a stab at updating the variablesWithPrefix function to encapsulate the logic around prefixes, comment formats and !default. Let me know what you think!

Hey all, I'm working on adding a custom transform for my DS for this feature and found an edge case with my stuff that I thought I would share because It looks like you might run into the same issue here with the current code. The edge case is when I have this:

// token json file "content-padding": { "comment": "The padding around the main content icon and message in the Alert component.", "value": "{space.m.value} {space.m.value} {space.m.value} {space.misc-a.value}", "type": "settings", "themable": true },

It outputs this:

$sprk-alert-content-padding: 16px !default 16px !default 16px !default 24px !default; // The padding around the main content icon and message in the Alert component.

The !default gets repeated unless we check for that case and make sure not to repeat it. Thought this might be helpful to y'all! Hope you are having a great week!

dbanksdesign · 2019-12-04T19:24:02Z

__tests__/formats/scssVariables.test.js

+      themeableScss = formatter(themeableDictionary);
+
+      expect(themeableScss).toMatch("#EF5350 !default;");
+      return done();


We don't need return done(); here, that is used if the test has some asynchronous code which we do for testing scss/less validity.

Good to know! I should have looked more closely at what that was for in the other test.

This has been resolved.

dbanksdesign

LGTM!

jmmcduffie · 2020-01-09T16:57:04Z

Awesome! Thank you so much for reviewing this.

kevinmpowell · 2020-01-22T22:16:10Z

+1 for this feature. I'm just getting started w/style dictionary. Good to see @jmmcduffie making it better. Nice work!

jmmcduffie · 2020-01-25T21:01:37Z

Thanks, @kevinmpowell! I'll be curious to hear what you think of Style Dictionary.

chazzmoney · 2020-01-29T18:57:45Z

I think this is a really good direction and important functionality. However, I have two concerns:

The themeable property is only applicable to one format. I think it might have general use in languages or spaces where a default syntax exists. If we can find other places (especially with currently supported languages) where we can use this to generate syntactically appropriate defaults, I think that would be a very large value to the commit.
I'm worried about adding this without doing the previous. I realize this is a lot of extra work and maybe even outside your/our specialties, but we risk polluting the property space with single-format relevant properties. If we can aim to ensure property relevance to all formats, that would be preferred.

Thoughts?

jmmcduffie · 2020-01-29T19:26:01Z

Your concerns make a ton of sense, @chazzmoney.

My hope for this new property was exactly what you suggested in your first point, that other platforms would be able to use the same data with a different implementation. By giving it an open-ended name, my thought was that it would not be viewed as a single-format property and allow new use cases to arise.

But I totally get your worry about moving forward without knowing if that will really happen. And I also don't disagree that a better understanding of that would add value to this PR. Do you think that we could still provide that value by doing just enough research to validate that another format could use this?

My concern in lumping them together would be that we bloat the scope of this change. And if we determine that it truly is a single-format property, we can course-correct my implementation into something more appropriate.

chazzmoney · 2020-10-08T16:52:38Z

For those who are looking for someplace to help out - if you know if there are tags for defaults in the main library formats, we would love to have them added to this.

dbanksdesign · 2020-10-08T18:04:19Z

We added this PR to the 3.0 branch targeting the next major release (later this year) because it is adding something to the core token object itself. Before releasing 3.0 we want to see how this flag can work in other built-in formats and write some documentation about it.

feat: add support for !default in SCSS variables format

12b7754

For any token definition that includes the property `themeable: true`, a `!default` will now be added in the scss/variables format. GitHub issue amzn#307

jmmcduffie force-pushed the jmm/issue-37_optional-default-flag branch from f0e837e to 12b7754 Compare November 25, 2019 13:19

dbanksdesign reviewed Dec 4, 2019

View reviewed changes

fix: address feedback from code review

e98f4a2

dbanksdesign approved these changes Jan 8, 2020

View reviewed changes

chazzmoney added hacktoberfest help wanted labels Oct 8, 2020

chazzmoney added this to the 3.0 milestone Oct 8, 2020

dbanksdesign changed the base branch from master to 3.0 October 8, 2020 17:02

dbanksdesign merged commit fa82002 into amzn:3.0 Oct 8, 2020

jmmcduffie deleted the jmm/issue-37_optional-default-flag branch October 8, 2020 17:36

This was referenced Oct 8, 2020

Themeable in formats #474

Open

Sass variables should (optionally?) have !default flag #307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for !default in SCSS variables format #359

feat: add support for !default in SCSS variables format #359

jmmcduffie commented Nov 23, 2019 •

edited

Loading

dbanksdesign left a comment

dbanksdesign Dec 4, 2019

jmmcduffie Dec 5, 2019 •

edited

Loading

jmmcduffie Dec 5, 2019

afebbraro Oct 8, 2020 •

edited

Loading

dbanksdesign Dec 4, 2019

jmmcduffie Dec 4, 2019

jmmcduffie Dec 5, 2019

dbanksdesign left a comment

jmmcduffie commented Jan 9, 2020

kevinmpowell commented Jan 22, 2020

jmmcduffie commented Jan 25, 2020

chazzmoney commented Jan 29, 2020

jmmcduffie commented Jan 29, 2020

chazzmoney commented Oct 8, 2020

dbanksdesign commented Oct 8, 2020

feat: add support for !default in SCSS variables format #359

feat: add support for !default in SCSS variables format #359

Conversation

jmmcduffie commented Nov 23, 2019 • edited Loading

dbanksdesign left a comment

Choose a reason for hiding this comment

dbanksdesign Dec 4, 2019

Choose a reason for hiding this comment

jmmcduffie Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

jmmcduffie Dec 5, 2019

Choose a reason for hiding this comment

afebbraro Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

dbanksdesign Dec 4, 2019

Choose a reason for hiding this comment

jmmcduffie Dec 4, 2019

Choose a reason for hiding this comment

jmmcduffie Dec 5, 2019

Choose a reason for hiding this comment

dbanksdesign left a comment

Choose a reason for hiding this comment

jmmcduffie commented Jan 9, 2020

kevinmpowell commented Jan 22, 2020

jmmcduffie commented Jan 25, 2020

chazzmoney commented Jan 29, 2020

jmmcduffie commented Jan 29, 2020

chazzmoney commented Oct 8, 2020

dbanksdesign commented Oct 8, 2020

jmmcduffie commented Nov 23, 2019 •

edited

Loading

jmmcduffie Dec 5, 2019 •

edited

Loading

afebbraro Oct 8, 2020 •

edited

Loading