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

Add list-style-image support #10817

Merged
merged 8 commits into from
Mar 20, 2023
Merged

Add list-style-image support #10817

merged 8 commits into from
Mar 20, 2023

Conversation

adamwathan
Copy link
Member

@adamwathan adamwathan commented Mar 17, 2023

This PR adds support for the list-style-image property under the list-* namespace.

By default, only list-none is included, but custom values can be added using the listStyleImage theme configuration or using arbitrary values.


Implementation

The obvious way to implement this feature is by adding a new listStyleImage core plugin that handles the list-style-image property, but it creates a documentation challenge because there are no sensible default values for us to include (since images like this are very project specific), which would basically make the documentation page empty, or at least have no table at the top. This would be the very first time that the documentation for a utility didn't include any sort of reference table which felt like a smell and led us to look for other ways to approach this feature.

For a while we were convinced that the best way to implement this feature would actually be to deprecate the existing listStyleType utilities in favor of a generic listStyle plugin that targeted the list-style shorthand property in CSS. There's not a lot of point in setting both list-style-type and list-style-image on an element, so combining these into a single abstraction in Tailwind felt like a nice convenience to the end user, and would allow them to configure custom types and images under a single key in their tailwind.config.js file.

We worked on this for quite a while but ultimately decided against that approach for two reasons:

  1. Significant complexity needed for backwards compatibility. We take backwards compatibility pretty seriously and wanted to make sure that we could introduce a new listStyle plugin without forcing users to have to make any changes to their project to upgrade.

    This meant we needed to make sure customization theme values added to listStyleType were automatically inherited by the new listStyle plugin, that calls to theme('listStyleType.disc') still worked in both your CSS and within JS plugins, and that if you'd disabled the listStyleType core plugin that the listStyle plugin would be disabled. On top of this, we wanted to issue warnings for anyone interacting with the old APIs to motivate them to replace their use of listStyleType with listStyle.

    The more we worked on this, the more nasty edge cases we found where the correct behavior felt very hard to decide on, and where handling even the edge cases we did know how to handle was introducing lots of nasty conditional code throughout many different places in the codebase. This just felt like a smell and forced us to step back and consider if there were any other solutions.

  2. The list-style property doesn't compose with list-style-position. The list-style shorthand property is short for list-style-type, list-style-image, and list-style-position. That means if you set list-style after setting any of those properties, those properties are overridden.

    This shows up as a problem when applying classes conditionally, for example:

    <ul class="list-disc list-inside md:list-decimal">
      <!-- ... -->
    </ul>

    In the above example, if we used list-style to implement list-decimal, then md:list-decimal would override the list-style-position set by list-inside. In practice this probably isn't a big deal because it's pretty strange to change the bullet style conditionally, but it was a signal that using the generic list-style property was probably not the right solution.

Ultimately we decided to go with the first implementation idea which is a dedicated listStyleImage plugin, but solve the documentation problem by combining both listStyleImage and listStyleType into a single documentation page called "List Style Type / Image", much like we do with the grid-column, grid-column-start, and grid-column-end utilities.

We also decided to include none as a default value for listStyleImage just to make sure things like this would work:

<ul class="list-cupcake md:list-none">
  <!-- ... -->
</ul>

Because listStyleType also includes none as a default value, this means that using list-none in your markup will generate two classes:

.list-none {
  list-style-type: none;
}
.list-none {
  list-style-image: none;
}

We considered writing some special case code here so that we could just include none under listStyleType but automatically use list-style instead of list-style-type just for the none value, but that would have the same issues I mentioned in point 2 above where it breaks composability. It also just feels dirty to special case things in the codebase, and is usually a sign that we're doing something we shouldn't be doing.

One minor downside to list-none working this way is that if for some reason you really want to set both a type and an image but unset just the type or image at a different breakpoint, you can't, because list-none will always unset both:

<!-- Will unset both the type and image -->
<ul class="list-disc list-cupcake md:list-none">

Even respecifying the one you want to keep won't work, because it will collide with the version of list-none that comes from that same core plugin:

<!-- Still won't work -->
<ul class="list-disc list-cupcake md:list-none md:list-cupcake">

This feels like something we need to solve before merging, as it's another one of those weird smells that just tells me our approach here isn't right. So far, I have two ideas but am not in love with either of them:

  1. Use different names. Maybe list-none is for list-style-type for BC reasons, and list-image-none or list-no-image is used for list-style-image. Would work, name options just suck.

  2. Special case list-none. Maybe we add list-none using addUtilities so it is always sorted before custom values. This would work but it would be the first time we've mixed addUtilities and matchUtilities in the same plugin, and any time we do something for the first time we need to think carefully about if we want to set that precedent.

Going to leave this open for now and probably waste four more hours talking about it with @reinink when he's back from vacation on Monday.

@adamwathan
Copy link
Member Author

adamwathan commented Mar 19, 2023

Note to self — the line-clamp plugin actually does do the thing I talked about where we mix usage of matchUtilities and addUtilities in the same plugin:

https://github.com/tailwindlabs/tailwindcss-line-clamp/blob/master/src/index.js#L10-L48

...but in this case it's not clear which plugin we should be defining list-none in. I guess maybe both? The line-clamp stuff is an interesting precedent for hardcoding the none value though while deriving the rest of the values from the config. Maybe we just do that, fuck it?

Main issue would be that none wouldn't be included in the config and thus theme('listStyleType.none') wouldn't work. There is literally zero reason to ever do that but it would technically be a breaking change. If a breaking change affects zero users though did it really break anything? 🌲

Could work around that by filtering out the none value from the config explicitly in the plugin, so the none value can still exist in the config but be ignored by the matchUtilities call.

Really basic implementation for Monday-Adam's benefit, without the none filtering because I'm doing this from the bath tub and I'm getting wrinkly:

  listStyleType: (args) => {
    args.addUtilities({
      '.list-none': { 'list-style-type': none },
    })
    createUtilityPlugin('listStyleType', [['list', ['listStyleType']]])(args)
  },
  listStyleImage: (args) => {
    args.addUtilities({
      '.list-none': { 'list-style-image': none },
    })
    createUtilityPlugin('listStyleImage', [['list', ['listStyleImage']]], {
      type: ['lookup', 'image', 'url'],
    })(args)
  },

@adamwathan
Copy link
Member Author

Talked to @reinink — just going to put all the list-style-image stuff under the list-image-* utility namespace and avoid all these collisions. Bit longer but so much simpler and less likely to run into some unexpected side effect that makes us want to delete the repository.

Nice benefit is you don't need to type-hint image when using a CSS variable as an arbitrary value:

- <div class="list-[image:var(--my-var)]">
+ <div class="list-image-[var(--my-var)]">

Same number of characters but people won't need to read the docs to figure it out.

So to summarize the new changes:

  • New listStyleImage plugin, pulls from listStyleImage theme key
  • Only default utility is list-image-none
  • Can use arbitrary values like list-image-[url(./my-image.png)]

So yeah that was a lot of effort for basically nothing.

@adamwathan adamwathan force-pushed the feat/add-list-style-utilities branch from 9298e59 to 22afcd5 Compare March 20, 2023 18:35
@adamwathan adamwathan merged commit bac5ecf into master Mar 20, 2023
@adamwathan adamwathan deleted the feat/add-list-style-utilities branch March 20, 2023 18:40
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.

None yet

2 participants