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

Allow configs to override default CSS theme values in theme() function provided to plugins and configs #14359

Merged
merged 12 commits into from
Sep 6, 2024

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Sep 6, 2024

Previously, given the following CSS and configuration:

/* app.css */

@theme default {
  --font-size-base: 1.25rem;
  --font-size-base--line-height: 1.5rem;
}
@tailwind utilities;
@config "./config.js";
// config.js

export default {
  theme: {
    fontSize: {
      // …
      base: ['1rem', { lineHeight: '1.75rem' }],
    },

    // …
  },
};

When a config or a plugin asked for the value of theme(fontSize.base) like so:

// config.js
export default {
  theme: {
    // …

    typography: ({ theme }) => ({
      css: {
        '[class~="lead"]': {
          fontSize: theme('fontSize.base')[0],
          ...theme('fontSize.base')[1],
        },
      }
    }),
  },
};

We would instead pull the values from the CSS theme even through they're marked with @theme default. This would cause the incorrect font size and line height to be used resulting in something like this (in the case of the typography plugin with custom styles):

.prose [class~="lead"] {
  font-size: 1.25rem;
  line-height: 1.5rem;
}

After this change we'll now pull the values from the appropriate place (the config in this case) and the correct font size and line height will be used:

.prose [class~="lead"] {
  font-size: 1rem;
  line-height: 1.75rem;
}

This will work even when some values are overridden in the CSS theme:

/* app.css */
@theme default {
  --font-size-base: 1.25rem;
  --font-size-base--line-height: 1.5rem;
}
@theme {
  --font-size-base: 2rem;
}
@tailwind utilities;
@config "./config.js";

which would result in the following CSS:

.prose [class~="lead"] {
  font-size: 2rem;
  line-height: 1.75rem;
}

[key: string | number]: string | Colors
}

export function flattenColorPalette(colors: Colors) {
Copy link
Member

Choose a reason for hiding this comment

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

TODO: We need to expose this to the end user for backwards compatibility. I believe it's tailwindcss/lib/util/flattenColorPalette

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really unfortunate it needs to live at tailwindcss/lib/util/flattenColorPalette and not tailwindcss/flattenColorPalette.

Do you think we should do this in this PR or a followup?

Copy link
Member

Choose a reason for hiding this comment

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

Can that be something we configure in our exports config or copy over at build-time or something I wonder?

Copy link
Member

@RobinMalfait RobinMalfait Sep 6, 2024

Choose a reason for hiding this comment

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

I think we would still require a separate file to point to, but the paths should be fixable via exports I believe.

I also think that it should be a default export in the final result so that you can do const flattenColorPalette = require('...') which is what's currently used.

@thecrypticace thecrypticace marked this pull request as ready for review September 6, 2024 19:39
@adamwathan adamwathan force-pushed the fix/theme-callback-tuple-precedence branch from cea86ed to 14555ea Compare September 6, 2024 19:44
@adamwathan adamwathan merged commit b1e22e1 into next Sep 6, 2024
3 checks passed
@adamwathan adamwathan deleted the fix/theme-callback-tuple-precedence branch September 6, 2024 20:05
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