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(custom-properties): use default key name if pluralize returns empty #2389

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

wahidrahim
Copy link
Contributor

This fixes a bug in custom-properties where keys such as "s" are not generated correctly. This is due to the pluralize package not handling "s" in an expected way in this context of themes.

Essentially

pluralize("s", 1) === ""

This will not be corrected on pluralize, see: plurals/pluralize#36

This poses a problem for theme keys which is using "s" as a key to indicate "small", for example.

Bug

Consider a theme where we have radius sizes defined as:

const theme: Theme = {
  // ...
  radii: {
    0: '0px',
    s: '2px',
    m: '4px',
    l: '8px',
    xl: '16px',
  },
}

This will create an incorrect CSS var, for the "s" key:

--theme-ui-radius-0: 0px;
--theme-ui-radius-: 2px; /* Incorrect */
--theme-ui-radius-m: 4px;
--theme-ui-radius-l: 8px;
--theme-ui-radius-xl: 16px;

And you also get the invalid CSS custom property warning:

theme-ui] Theme key "2px" found will produce an invalid CSS custom property. Keys must only contain the following: A-Z, a-z, 0-9, hyphen, underscore.

Solution

Since pluralize can at times return empty strings, in such cases it is better to just use the key itself. With this fix the output for the radius CSS custom properties are as expected:

--theme-ui-radius-0: 0px;
--theme-ui-radius-s: 2px;
--theme-ui-radius-m: 4px;
--theme-ui-radius-l: 8px;
--theme-ui-radius-xl: 16px;

@vercel
Copy link

vercel bot commented Feb 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
theme-ui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 3, 2023 at 7:57PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 3, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 91f14e9:

Sandbox Source
next-theme-ui-example Configuration
gatsby-plugin-theme-ui-example Configuration

Copy link
Member

@lachlanjc lachlanjc left a comment

Choose a reason for hiding this comment

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

Thank you for the thorough info & tests! Looks great

@hasparus hasparus merged commit 4c84e7d into system-ui:develop Feb 7, 2023
@hasparus hasparus added patch Increment the patch version when merged prerelease This change is available in a prerelease. labels Feb 7, 2023
@hasparus hasparus mentioned this pull request Feb 7, 2023
@hasparus
Copy link
Member

hasparus commented Feb 7, 2023

🚀 PR was released in v0.15.5 🚀

@hasparus hasparus added released This issue/pull request has been released. and removed prerelease This change is available in a prerelease. labels Feb 7, 2023
@wahidrahim wahidrahim deleted the fix-custom-properties branch February 8, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants