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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Oct 8, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12b7754
feat: add support for !default in SCSS variables format
jmmcduffie Nov 23, 2019
e98f4a2
fix: address feedback from code review
jmmcduffie Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions __tests__/formats/scssVariables.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var formats = require('../../lib/common/formats');
var scss = require('node-sass');
var _ = require('lodash');

var file = {
"destination": "__output/",
Expand Down Expand Up @@ -62,5 +63,18 @@ describe('formats', () => {
});
});

it('should optionally use !default', done => {
var themeableDictionary = _.cloneDeep(dictionary),
formattedScss = formatter(dictionary),
themeableScss = "";

expect(formattedScss).not.toMatch("!default");

themeableDictionary.allProperties[0].themeable = true;
themeableScss = formatter(themeableDictionary);

expect(themeableScss).toMatch("#EF5350 !default;");
return done();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been resolved.

});
});
});
15 changes: 12 additions & 3 deletions lib/common/formats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,14 @@ function fileHeader(options, commentStyle) {

function variablesWithPrefix(prefix, properties, commentStyle) {
return properties.map(function(prop) {
var to_ret_prop = prefix + prop.name + ': ' + (prop.attributes.category==='asset' ? '"'+prop.value+'"' : prop.value) + ';';
var to_ret_prop = prefix + prop.name + ': ';
to_ret_prop += (prop.attributes.category==='asset' ? '"'+prop.value+'"' : prop.value);

if (prop.themeable === true) {
to_ret_prop += ' !default';
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@jmmcduffie jmmcduffie Dec 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link

@afebbraro afebbraro Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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!

}

to_ret_prop += ';';

if (prop.comment) {
if (commentStyle === 'short') {
Expand Down Expand Up @@ -175,14 +182,16 @@ module.exports = {
},

/**
* Creates a SCSS file with variable definitions based on the style dictionary
* Creates a SCSS file with variable definitions based on the style dictionary.
*
* Add `!default` to any variable by setting a `themeable: true` property in the token's definition.
*
* @memberof Formats
* @kind member
* @example
* ```scss
* $color-background-base: #f0f0f0;
* $color-background-alt: #eeeeee;
* $color-background-alt: #eeeeee !default;
* ```
*/
'scss/variables': function(dictionary) {
Expand Down