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

Default animation keyframes should respect prefix in config #2125

Closed
andrewbrey opened this issue Aug 4, 2020 · 15 comments
Closed

Default animation keyframes should respect prefix in config #2125

andrewbrey opened this issue Aug 4, 2020 · 15 comments

Comments

@andrewbrey
Copy link

Describe the problem:

Starting with v1.6.0, without any customization to the configuration there are animation utilities produced including @keyframes values. The keyframes generated have names which are pretty common (spin, ping, pulse, and bounce) and when bringing tailwind into a codebase with existing styles, there's a nontrivial chance of name conflict with these names. My hope would be that the generated keyframes would respect the config prefix value so that the names of the animation keyframes were, e.g. tw-spin, tw-pulse, etc.

Obviously these names can be adjusted by changing the keyframes configuration settings (so this is not a breaking issue or at least it has a straight forward workaround), but given that these are produced by default, I think they should respect the prefix.

Link to a minimal reproduction:

Using a stock npx tailwindcss init config file with the prefix set will reproduce this:

module.exports = {
  purge: true,
  prefix: 'tw-',
  theme: {
    extend: {},
  },
  variants: {},
  plugins: [],
};
@adamwathan
Copy link
Member

Interesting one for sure, I think I agree in principal but the challenge is how do we prefix the animation name reliably in the actual animation definition?

For example, given this config:

module.exports = {
  prefix: 'tw-',
  theme: {
    animation: {
      wiggle: 'wiggle 1s ease-in-out infinite',
    },
    keyframes: {
      wiggle: {
        // ...
      }
    }
  },
  // ...
};

...how do we prefix the instance of "wiggle" inside the 'wiggle 1s ease-in-out infinite' string?

Do we do a magic string substitution where we find any strings that match the name of a keyframe definition and prefix them? Is that too magic? Are there unintended side-effects of that? Do we have to make sure we only replace matches that are at the very beginning of the string? What if someone is deliberately trying to reference an animation they've defined in custom CSS and they don't want it prefixed? I guess only if it matches the keyframes name exactly we replace it? 🤔

Any thoughts on the most bullet proof way to do this that also follows the principle of least surprise?

@andrewbrey
Copy link
Author

Ya, I get what you mean about how might be best to blend a customized keyframes config with an extant prefix config. That said, by way of trying to see the forest for the trees, I think the easiest thing to do is just change the default names of the 4 keyframes included in the default config to be more unique (tailwind-spin?). Then the task of respecting your chosen prefix is on users who customize their keyframes config.

Thoughts?

@adamwathan
Copy link
Member

Only issue with that is it's a breaking change, since someone may have defined a new custom animation entry that is reference an existing keyframes rule 🙁 Otherwise would 100% agree, bah.

@andrewbrey
Copy link
Author

Only issue with that is it's a breaking change, since someone may have defined a new custom animation entry that is reference an existing keyframes rule slightly_frowning_face Otherwise would 100% agree, bah.

Gah, you are right haha - is there like a grace period where you can say "this has been in the wild less than 2 weeks, no complaints plz 😬"

One option would be to introduce a second set (identical definitions, just updated names), mark the old as deprecated and move on. If someone is using the "old" names (spin, ping, etc) and they have no conflict with the names, you don't have to care. If they are using the old names and they have a conflict with the names, they will have a very simple remediation available in switching to the "new" names (tailwind-spin, etc). Ship both for two minor releases and then axe the old names - it's perfect 😄

@adamwathan
Copy link
Member

If we deprecated the names how would that fix the collision issue for people using the prefix if we still include them? 🤔

@andrewbrey
Copy link
Author

The intention of shipping both for a time wouldn't be to fix the collision, it would be to provide a small transition window for users to become aware of the potential for collision and make the (almost certainly tiny) updates required to de-collide themselves by converting to the new names. This is a time when semver rules seems silly since this is ultimately a pretty small change that if you just made it would take most if not all people affected like 5 min to adjust to :/

@huphtur
Copy link

huphtur commented Aug 23, 2020

Just noticed that even with purge mode: 'all', enabled, the keyframes are still included, is that expected?

@adamwathan
Copy link
Member

adamwathan commented Aug 23, 2020

It is the default in PurgeCSS yeah, you need to add keyframes: true to your PurgeCSS options to remove unused keyframes:

module.exports = {
  purge: {
    options: {
      keyframes: true,
    }
  }
}

https://purgecss.com/configuration.html#options

@huphtur
Copy link

huphtur commented Aug 23, 2020

Ah neat, I was actually removing them by disabling the animation property.

@AndrewBogdanovTSS
Copy link

A little bit of a side question, is there a way to prefix only animations and nothing else? I ran into an issue with TW animations overwrite by another lib since it's also using a pulse animation name, so I would rather call mine tw-pulse

@adamwathan
Copy link
Member

Solved for Tailwind 2.0, prefix will apply to the keyframe name and automatically within the animation value as well (we wrote a custom animation value parser), but it will skip prefixing animation names that don't match ones in your keyframes config.

#2621
#2641

@AndrewBogdanovTSS
Copy link

@adamwathan will it require to use a prefix option that will prefix all classes? Is there a way to only prefix animations and nothing else?

@adamwathan
Copy link
Member

You can override the animations in your config file and name them whatever you want 👍🏻

@AndrewBogdanovTSS
Copy link

@adamwathan but that will mean creating duplicated animations that do the same thing?
Currently I do it like this:

tailwind.config.js

module.exports = {
  theme: {
    extend: {
     keyframes: {
        'tw-pulse': {
          '0%, 100%': {opacity: 1},
          '50%': {opacity: 0.5}
        }
      },
     animation: {
        'tw-pulse': 'tw-pulse 2s cubic-bezier(0.4, 0, 0.6, 1) infinite'
      }   
    }
}

Or am I missing something?

@adamwathan
Copy link
Member

As long as you don't put them under extend you won't end up with duplicates:

module.exports = {
  theme: {
    keyframes: {
      'tw-pulse': {
        '0%, 100%': {opacity: 1},
        '50%': {opacity: 0.5}
      }
    },
    animation: {
      'tw-pulse': 'tw-pulse 2s cubic-bezier(0.4, 0, 0.6, 1) infinite'
    }   
  }
}

But yes you will have to write out the values you want, and if you want to use the same values we use by default then you'll have to write those out or pull them in from tailwindcss/defaultTheme.

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

No branches or pull requests

4 participants