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.

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(formats): Add outputReferences support to scss/map-deep #720

Merged
merged 5 commits into from
Nov 9, 2021

Conversation

notlee
Copy link
Contributor

@notlee notlee commented Oct 19, 2021

Use the formattedVariables function within the scss/map-deep formatter
to add support for the outputReferences option.
Related issue: #712

before:

$color-core-neutral-0: #ffffff !default;
$color-background-primary: #ffffff !default;

$design-system-tokens: (
  'color': (
    'background': (
      'primary': $color-background-primary
    ),
    'neutral': (
      '0': $color-core-neutral-0,
    )
  )
)

after:

$color-core-neutral-0: #ffffff !default;
$color-background-primary: $color-core-neutral-0 !default;

$design-system-tokens: (
  'color': (
    'background': (
      'primary': $color-background-primary
    ),
    'neutral': (
      '0': $color-core-neutral-0,
    )
  )
)

A side effect of this change is that scss/map-deep also
supports the themeable token property. For backward compatibility
changes have been made so tokens default to being themeable
with scss/map-deep, unlike for scss/variables where tokens
are not themeable by default. See #474

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

Use the `formattedVariables` within the `scss/map-deep` formatter
to add support for the `outputReferences` option.
Closes: amzn#712

A side effect of this change is that `scss/map-deep` also
supports the `themeable` token property. For backward compatibility
changes have been made so tokens default to being themeable
with `scss/map-deep`, unlike for `scss/variables` where tokens
are not themeable by default. See amzn#474
@notlee
Copy link
Contributor Author

notlee commented Oct 19, 2021

I'm using an internal themeable_default argument here to maintain the default behaviour of scss/map-deep but we could also pass through a themeable option on the formatter if we wanted.

@notlee notlee mentioned this pull request Oct 19, 2021
Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

This PR is great! Just 2 minor comments about the themeable_default and snapshots and I think we are good!

lib/common/formats.js Outdated Show resolved Hide resolved
__tests__/formats/__snapshots__/scssMaps.test.js.snap Outdated Show resolved Hide resolved
__integration__/scss.test.js Show resolved Hide resolved
An option to add/remove the `!default` flag on Sass variables by default.
This may be overridden by setting a `themeable` attribute in a token's
definition.

Defaults to `true` for backward compatibility. Unlike the
`scss/variables` which does not output Sass variables with the
`!default` flag by default.
- remove trailing new line
- add leading new line
Whether or not to add the `!default` flag on Sass variables by default.
This may be overridden by setting a `themeable` attribute in a token's
definition.

Matches the `themeable` option of the `scss/map-deep` format but
defaults to `false`, retaining its existing behaviour.
seems kebab case is used more than camel case
@notlee notlee requested a review from dbanksdesign October 26, 2021 12:22
Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making those changes!

@dbanksdesign dbanksdesign merged commit 65453e0 into amzn:main Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants