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

fix(docs): change TOKEN_COLUMNS to avoid conflicting tokens with the same name #1736

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

rssilva
Copy link
Contributor

@rssilva rssilva commented Sep 18, 2024

Description of the change

  • Add a name property to TokensTable
  • Add a null check to TokensTable to avoid breaking by accessing an undefined value
  • Change TOKENS_TABLE to have one more level (or prefix) to each object. So a titleDisplay that comes from font-size would not collide with titleDisplay that comes from font-weight
  • Call camelcase on tokens names avoiding things as colorBackgrounddecorative-strong correcting it to colorBackgroundDecorativeStrong

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • Non-Breaking Change (change to existing functionality)

BEFORE

Screen Shot 2024-09-18 at 08 50 40
Screen Shot 2024-09-18 at 08 50 31

AFTER

Screen Shot 2024-09-18 at 08 50 22
Screen Shot 2024-09-18 at 08 50 48

Copy link

changeset-bot bot commented Sep 18, 2024

🦋 Changeset detected

Latest commit: 1c3d017

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@localyze-pluto/design-tokens Patch
@localyze-pluto/components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rssilva rssilva force-pushed the fix-avoid-docs-conflicting-tokens branch from 43f2746 to 8738fb2 Compare September 18, 2024 11:53
Copy link
Contributor

github-actions bot commented Sep 18, 2024

Size Change: 0 B

Total Size: 43 kB

ℹ️ View Unchanged
Filename Size
packages/components/dist/index.js 25.7 kB
packages/design-tokens/dist/index.js 8.91 kB
packages/design-tokens/dist/js/border-radius.js 181 B
packages/design-tokens/dist/js/border-style.js 140 B
packages/design-tokens/dist/js/border-width.js 146 B
packages/design-tokens/dist/js/colors.js 4.44 kB
packages/design-tokens/dist/js/font-family.js 141 B
packages/design-tokens/dist/js/font-size.js 260 B
packages/design-tokens/dist/js/font-weight.js 156 B
packages/design-tokens/dist/js/line-height.js 193 B
packages/design-tokens/dist/js/margin.js 131 B
packages/design-tokens/dist/js/padding.js 159 B
packages/design-tokens/dist/js/size.js 247 B
packages/design-tokens/dist/js/space.js 549 B
packages/design-tokens/dist/js/z-index.js 168 B
packages/theme/dist/index.js 1.47 kB

compressed-size-action

Copy link
Contributor

@apvale apvale left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏🏻

@@ -105,7 +107,7 @@ const FONT_SIZE = reduce(
name: "Name",
transform: getTokenNameFromTuple(prefix),
},
{ name: "Pixels", transform: getTokenComment },
{ name: "Weight", transform: getTokenValue },
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use Weight only for font-weight?

image

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'm removing this column since it doesn't make sense on font size!

@@ -18,15 +18,17 @@ import { TokenName } from "./TokenName";

export const TokensTable = ({
data,
name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we are using category in the type. What do you think of also using it here? Or maybe changing there to name so we have the same naming in both places?

Copy link
Contributor Author

@rssilva rssilva Sep 18, 2024

Choose a reason for hiding this comment

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

good call! I'm changing to category

@rssilva rssilva merged commit cc677ad into main Sep 18, 2024
14 checks passed
@rssilva rssilva deleted the fix-avoid-docs-conflicting-tokens branch September 18, 2024 17:55
@alpacatron alpacatron mentioned this pull request Sep 18, 2024
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.

2 participants