Skip to content

Conversation

@adamwathan
Copy link
Member

This PR updates all of our x/y named utilities (like px-*, my-*, inset-y-*, scroll-px-*, etc.) to use logical properties like padding-inline instead of separate padding-left and padding-right properties.

We held off on this originally for a while because inline doesn't really mean horizontal like the "x" in px-* implies, but I think in practice this change is fine — I'm comfortable with "x" meaning "in the inline direction" and "y" meaning "in the block direction" in Tailwind.

This is technically a breaking change if anyone was depending on the fact that px-* for instance was always explicitly setting padding-left and padding-right even when building something in a vertical writing mode, but I kinda doubt there's a single real project on the internet that will actually be affected, so not too worried about it.

If someone really wants to set padding-left and padding-right they can always use pl-4 pr-4 instead of px-4.

Nice thing about this change is it produces quite a bit less CSS.

To test this change, I re-generated all of the snapshots and made sure none of the generated utilities changed position or anything in the output (initially they did before I updated property-order.ts to add some missing properties).

I also created a little demo locally in the Vite playground to test things manually and make sure I didn't make any obvious typos or anything that could slip through the snapshots:

image
Show code for playground demo
import React from 'react'

export function App() {
  return (
    <div className="p-12 gap-10 grid grid-cols-2 items-start">
      <div className="grid grid-cols-1 gap-10 justify-start">
        <div className="space-y-6">
          <p className="font-semibold mb-6">Border Width</p>
          <div className="border-x w-48 h-12 flex items-center justify-center">border-x</div>
          <div className="border-y w-48 h-12 flex items-center justify-center">border-y</div>
        </div>
        <div className="space-y-6">
          <p className="font-semibold mb-6">Border Color</p>
          <div className="border-2 border-x-red-500 w-48 h-12 flex items-center justify-center">
            border-x-red-500
          </div>
          <div className="border-2 border-y-red-500 w-48 h-12 flex items-center justify-center">
            border-y-red-500
          </div>
        </div>
        <div className="space-y-6">
          <p className="font-semibold mb-6">Padding</p>
          <div>
            <div className="px-8 bg-yellow-300 inline-flex items-center justify-center">px-8</div>
          </div>
          <div>
            <div className="py-8 bg-yellow-300 inline-flex items-center justify-center">py-8</div>
          </div>
        </div>
      </div>
      <div className="grid grid-cols-1 gap-10 justify-start">
        <div className="space-y-6">
          <p className="font-semibold mb-6">Margin</p>
          <div>
            <div className="bg-red-400 inline-flex">
              <div className="mx-8 bg-yellow-300 inline-flex items-center justify-center">mx-8</div>
            </div>
          </div>
          <div>
            <div className="bg-red-400 inline-flex">
              <div className="my-8 bg-yellow-300 inline-flex items-center justify-center">my-8</div>
            </div>
          </div>
        </div>
        <div className="space-y-6">
          <p className="font-semibold mb-6">Inset</p>
          <div className="relative bg-red-400 w-48 h-48">
            <div className="inset-x-8 absolute bg-yellow-300 inline-flex items-center justify-center">
              inset-x-8
            </div>
          </div>
          <div className="relative bg-red-400 w-48 h-48">
            <div className="inset-y-8 absolute bg-yellow-300 inline-flex items-center justify-center">
              inset-y-8
            </div>
          </div>
        </div>
      </div>
    </div>
  )
}

I didn't manually test the scroll padding or scroll margin utilities because they are more annoying to set up, but I probably should.

@adamwathan adamwathan requested a review from a team as a code owner October 27, 2024 19:26
@RobinMalfait
Copy link
Member

Not introduced in this PR, but it looks like we already use inline start/end on space-x utilities. I wonder if we can further simplify this.

functionalUtility('space-x', {
supportsNegative: true,
themeKeys: ['--space', '--spacing'],
handle: (value) => [
atRoot([property('--tw-space-x-reverse', '0', '<number>')]),
rule(':where(& > :not(:last-child))', [
decl('--tw-sort', 'row-gap'),
decl('margin-inline-start', `calc(${value} * var(--tw-space-x-reverse))`),
decl('margin-inline-end', `calc(${value} * calc(1 - var(--tw-space-x-reverse)))`),
]),
],
})

I think this should be the equivalent:

  functionalUtility('space-x', {
    supportsNegative: true,
    themeKeys: ['--space', '--spacing'],
    handle: (value) => [
      atRoot([property('--tw-space-x-reverse', '0', '<number>')]),

      rule(':where(& > :not(:last-child))', [
        decl('--tw-sort', 'row-gap'),
-       decl('margin-inline-start', `calc(${value} * var(--tw-space-x-reverse))`),
-       decl('margin-inline-end', `calc(${value} * calc(1 - var(--tw-space-x-reverse)))`),
+       decl('margin-inline', `calc(${value} * calc(1 - var(--tw-space-x-reverse)))`),
      ]),
    ],
  })

Even if we don't apply the space-x simplification, I think we should at least update space-y to use the same kind of properties, in this case using margin-block.

functionalUtility('space-y', {
supportsNegative: true,
themeKeys: ['--space', '--spacing'],
handle: (value) => [
atRoot([property('--tw-space-y-reverse', '0', '<number>')]),
rule(':where(& > :not(:last-child))', [
decl('--tw-sort', 'column-gap'),
decl('margin-top', `calc(${value} * var(--tw-space-y-reverse))`),
decl('margin-bottom', `calc(${value} * calc(1 - var(--tw-space-y-reverse)))`),
]),
],
})

Updating this with margin-block-* instead could look like this:

  functionalUtility('space-y', {
    supportsNegative: true,
    themeKeys: ['--space', '--spacing'],
    handle: (value) => [
      atRoot([property('--tw-space-y-reverse', '0', '<number>')]),

      rule(':where(& > :not(:last-child))', [
        decl('--tw-sort', 'column-gap'),
-       decl('margin-top', `calc(${value} * var(--tw-space-y-reverse))`),
-       decl('margin-bottom', `calc(${value} * calc(1 - var(--tw-space-y-reverse)))`),
+       decl('margin-block-start', `calc(${value} * var(--tw-space-y-reverse))`),
+       decl('margin-block-end', `calc(${value} * calc(1 - var(--tw-space-y-reverse)))`),
      ]),
    ],
  })

Or, with a similar change I suggested for space-x utilities:

  functionalUtility('space-y', {
    supportsNegative: true,
    themeKeys: ['--space', '--spacing'],
    handle: (value) => [
      atRoot([property('--tw-space-y-reverse', '0', '<number>')]),

      rule(':where(& > :not(:last-child))', [
        decl('--tw-sort', 'column-gap'),
-       decl('margin-top', `calc(${value} * var(--tw-space-y-reverse))`),
-       decl('margin-bottom', `calc(${value} * calc(1 - var(--tw-space-y-reverse)))`),
+       decl('margin-block', `calc(${value} * calc(1 - var(--tw-space-y-reverse)))`),
      ]),
    ],
  })

@adamwathan
Copy link
Member Author

I think this should be the equivalent:

Can't just use margin-inline here because that will set the same value for both start and end, and one of them is supposed to be zero depending on the value of --tw-space-x-reverse:

.space-x-4 {
  /* Will be `0` without using `space-x-reverse` */
  margin-inline-start: calc(1rem * var(--tw-space-x-reverse));

  /* Will be `0` when you _do_ use `space-x-reverse` */
  margin-inline-end: calc(1rem * calc(1 - var(--tw-space-x-reverse)));
}

Even if we don't apply the space-x simplification, I think we should at least update space-y to use the same kind of properties, in this case using margin-block.

Yeah I think we should do this, because gap-y-* already respects writing mode and switches to horizontal gaps with vertical writing modes, so feels like the space utilities should be consistent. Will update that.

@adamwathan adamwathan merged commit 289c25f into next Oct 28, 2024
1 check passed
@adamwathan adamwathan deleted the change/logical-xy branch October 28, 2024 17:08
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.

4 participants