fixed the naming for Sass/Scss in documentation, tests, formats #222

didoo · 2018-12-23T11:40:27Z

Issue #, if available:
#213

Description of changes:
I have renamed the sass/map-*** formats to scss/map-*** and updated some documentation.

We have to consider that this is technically a breaking change, so it's better to consider how to release it. One possibility is to include it in the next "major" release (3.0). One other is to release it as "minor", but show an error message for the users that are using the "old" format name (I don't know if it's possible, maybe someone has better ideas? I'm open to suggestions)

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

…reaking changes

didoo · 2018-12-23T12:22:34Z

I have pushed a commit with a proposed solution to avoid breaking changes: the idea is to have the sass/map-*** as duplicates (do you know if it's possible to call another format directly, as a function, in formats.js, to avoid duplication of code?), but add a warning message that alerts the user that soon will be removed. If it's ok we can keep it, otherwise I can easily revert it.

dbanksdesign · 2019-01-04T17:21:49Z

Instead of having a deprecation warning for 'sass/map-' formats, could we have a warning that those formats have moved to 'scss/map-' and say the sass formats will now be sass syntax? I still need to figure out how to do maps in sass syntax...

didoo · 2019-01-05T12:03:54Z

@dbanksdesign I have made this very rough test: I have extracted from https://adele.uxpin.com/ the design systems that declare are using Sass (filter in the column "CSS"); they are around 45.

I have then checked for all of them which syntax flavor they are using, sass or scss, and here are the results: https://docs.google.com/spreadsheets/d/17KYJwxR1hbfkT60Isq9v7ZOlPYZZBe7kjimLGsD1acQ/edit?usp=sharing

As you can see (and as I suspected) out of 40 design systems only 1 is using sass syntax, all the others are using scss. Why this exercise? To justify why, for me, we should avoid having/supporting a double format in Style Dictionary. Because almost everyone is using scss, and because having both would lead to inevitable confusion (how many would use sass when they actually mean scss, like I did?). I know is not statistically accurate, but I think this is a strong indicator for a KISS approach here.

# Conflicts: # __tests__/formats/__snapshots__/all.test.js.snap

dbanksdesign · 2019-01-08T17:24:59Z

Hmm I see your point. Thanks for compiling that spreadsheet. Not that we should be comparing style dictionary to theo, but theo does have sass and scss formats. Although they only have scss maps, and not sass maps.

I did finally figure out how to write maps in sass syntax:

$tokens: (color-background-base: #f0f0f0, color-background-alt: #eeeeee)

It has to be on 1 line because of the whitespace syntax I think. That might be why they don't do a sass map format?

I guess this discussion brings up a larger point of what is the bar to include transforms and formats in the framework itself? I don't have a good answer for that. I would lean towards including more if they are about language syntax, even if they are less popular languages. If it helps 2 people to not have to write their own format, I would say that is a win.

How about this, what if we didn't explicitly say "deprecate" in the warning message, but instead say that those formats moved to 'scss/map-'. We could merge this and then have a larger discussion about what formats should be included? Maybe we could create a new issue relating to sass formats and see the response, if it gets enough we add them in at that point?

chazzmoney · 2019-01-15T20:30:21Z

index.js

+    var sass_map_format_warnings = GroupMessages.flush(SASS_MAP_FORMAT_DEPRECATION_WARNINGS).join('\n  ');
+    console.log(chalk.bold.yellow(`
+⚠️ DEPRECATION WARNING ️️️️️⚠️
+The formats 'sass/map-***' have been renamed to 'scss/map-***', please update your config accordingly (will be soon removed).


Similar to Danny's thoughts, what if this said something like:

The formats 'sass/map-' have been renamed to 'scss/map-', please update your config. In the future 'sass/map-***' formats may output actual SASS instead of SCSS , which may break your current configuration.

Not sure what the exact right message is - @dbanksdesign thoughts?

@chazzmoney I like your message. if it's ok for @dbanksdesign I'll update it

Lets do it!

didoo · 2019-01-16T10:13:26Z

I would lean towards including more if they are about language syntax, even if they are less popular languages. If it helps 2 people to not have to write their own format, I would say that is a win.

Well, it depends:

how much is the (future) cost of maintenance of that extra feature? (imagine the impact of a refactoring, of fixing failing tests, etc)?
how much complexity does it add to the overall codebase/project?
how much makes things less clear for a new user?
etc

I tend to apply the Pareto principle in this kind of cases ;)

(also, I have seen too many open-source projects explode (and maintainers go in burnout) because of feature creep).

dbanksdesign · 2019-01-16T17:14:34Z

Very true. I have talked to some people about the idea of making style dictionary more like webpack, where people can publish extensions/plugins (or in our case transforms/formats/actions) so someone could publish a sass-style-dictionary package that adds sass specific transforms/formats. Then we wouldn't really need to worry about what we include in the core library as much. And actually, the node example PR is the first step to make that happen...

didoo · 2019-01-19T09:46:10Z

I have updated the message. This is how it looks like in the console now:

dbanksdesign

LGTM!

chazzmoney · 2019-01-29T20:43:28Z

@didoo I merged a bunch of things and the custom-format change caused a small conflict here. Can you update this one? Then we can merge.

Sorry for the inconvenience.

…ming

didoo · 2019-01-30T00:13:28Z

@chazzmoney strange, I pulled upstream/develop and merged it into my branch, and there were no conflicts. can you check if everything is OK now?

dbanksdesign · 2019-01-30T01:15:38Z

Looks good to me!

* feat: add registerFilter method (#233) * doc: remove registerTemplate from examples (#232) * feat: add node example (#228) * feat: making include accept an array of globs, like source does (#227) * fix: the naming for Sass/Scss in documentation, tests, formats, fixes #213 (#222) * fix: handle showFileHeader option in androids formats templates, fixes #237 (#243)

didoo added 3 commits December 23, 2018 11:13

fixed the naming for Sass/Scss in documentation, tests, formats

afac57b

possible implementation of a warning message for Sass maps to avoid b…

3035544

…reaking changes

added Jest tests snapshot

9e04ca3

Merge branch 'develop' into fix_sass_scss_naming

aaee2be

# Conflicts: # __tests__/formats/__snapshots__/all.test.js.snap

chazzmoney reviewed Jan 15, 2019

View reviewed changes

updated console message for sass/map-* formats

f11e8dc

dbanksdesign approved these changes Jan 22, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into fix_sass_scss_na…

d777b98

…ming

dbanksdesign merged commit 37006d9 into amzn:develop Jan 30, 2019

didoo deleted the fix_sass_scss_naming branch February 4, 2019 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed the naming for Sass/Scss in documentation, tests, formats #222

fixed the naming for Sass/Scss in documentation, tests, formats #222

didoo commented Dec 23, 2018 •

edited

Loading

didoo commented Dec 23, 2018 •

edited

Loading

dbanksdesign commented Jan 4, 2019

didoo commented Jan 5, 2019 •

edited

Loading

dbanksdesign commented Jan 8, 2019

chazzmoney Jan 15, 2019

chazzmoney Jan 15, 2019

didoo Jan 16, 2019

dbanksdesign Jan 16, 2019

didoo commented Jan 16, 2019 •

edited

Loading

dbanksdesign commented Jan 16, 2019

didoo commented Jan 19, 2019

dbanksdesign left a comment

chazzmoney commented Jan 29, 2019 •

edited

Loading

didoo commented Jan 30, 2019

dbanksdesign commented Jan 30, 2019

fixed the naming for Sass/Scss in documentation, tests, formats #222

fixed the naming for Sass/Scss in documentation, tests, formats #222

Conversation

didoo commented Dec 23, 2018 • edited Loading

didoo commented Dec 23, 2018 • edited Loading

dbanksdesign commented Jan 4, 2019

didoo commented Jan 5, 2019 • edited Loading

dbanksdesign commented Jan 8, 2019

chazzmoney Jan 15, 2019

Choose a reason for hiding this comment

chazzmoney Jan 15, 2019

Choose a reason for hiding this comment

didoo Jan 16, 2019

Choose a reason for hiding this comment

dbanksdesign Jan 16, 2019

Choose a reason for hiding this comment

didoo commented Jan 16, 2019 • edited Loading

dbanksdesign commented Jan 16, 2019

didoo commented Jan 19, 2019

dbanksdesign left a comment

Choose a reason for hiding this comment

chazzmoney commented Jan 29, 2019 • edited Loading

didoo commented Jan 30, 2019

dbanksdesign commented Jan 30, 2019

didoo commented Dec 23, 2018 •

edited

Loading

didoo commented Dec 23, 2018 •

edited

Loading

didoo commented Jan 5, 2019 •

edited

Loading

didoo commented Jan 16, 2019 •

edited

Loading

chazzmoney commented Jan 29, 2019 •

edited

Loading