tailwindcss icon indicating copy to clipboard operation
tailwindcss copied to clipboard

Add support for dimension prefix when using dynamic `min-*` and `max-*` (resolved sorting)

Open brandonmcconnell opened this issue 1 year ago β€’ 3 comments

UPDATE: I am opening this PR as a re-open of #11217, as I have accounted for the changes recommended by @adamwathan. I have fixed the sorting bug from before and provided feedback on his other questions at the bottom of this PR description under "Feedback resolution".


This PR adds support for using a new dimension prefix with dynamic min-* and max-* variants.

Syntax:

type-[length] or type-[dimension:length], where…

  • type: 'min' | 'max'
  • dimension: 'w' | 'h' (optional, can be excluded to omit and default to width)
  • length: any valid CSS length

Syntax examples:

/* min-[100px]   */ @media (min-width: 100px)
/* min-[w:100px] */ @media (min-width: 100px)
/* min-[h:100px] */ @media (min-height: 100px)
/* max-[100px]   */ @media (max-width: 100px)
/* max-[w:100px] */ @media (max-width: 100px)
/* max-[h:100px] */ @media (max-height: 100px)

Real use case examples:

  • only applied on screens taller than 100px

    <div class="min-[h:100px]:font-bold" />
    
  • only applied on screens taller and narrower than 100px

    <!-- without `w:` -->
    <div class="min-[h:100px]:max-[100px]:font-bold" />
    <!-- or with `w:` -->
    <div class="min-[h:100px]:max-[w:100px]:font-bold" />
    
  • only applied on screens shorter and narrower than 100px (no conflict between max-[w?:] and max-[h:])

    <!-- without `w:` -->
    <div class="max-[h:100px]:max-[100px]:font-bold" />
    <!-- or with `w:` -->
    <div class="max-[h:100px]:max-[w:100px]:font-bold" />
    
  • only applied on screens taller and wider than 100px (no conflict between min-[w?:] and min-[h:])

    <!-- without `w:` -->
    <div class="min-[h:100px]:min-[100px]:font-bold" />
    <!-- or with `w:` -->
    <div class="min-[h:100px]:min-[w:100px]:font-bold" />
    

Gotcha / usage note

Using w: is 100% optional and only exists so either dimension can be defined explicitly if the user so desires. If any prefix is used other than h: it defaults to using width:, though one consideration could be to throw a log.risk or log.warn if any non-''/'w:'/'h:' dimension prefix is used.

Feedback resolution

On the previous PR for this change (#11217), @adamwathan left some feedback re what this PR might need before re-opening. Here is his feedback as well as my resolution notes:

(expand/collapse @adamwathan's feedback)

Hey thanks @brandonmcconnell! I'm going to close this one for now for a couple reasons:

  1. I'm not sold on the new colon prefix syntax β€” I don't really want to add new syntax conventions to the framework for just one feature without really thinking hard about it, and in this case my instinct would be to introduce new variants like min-h-[...] and max-h-[...] instead which feels a lot simpler.

  2. There are issues with the implementation that prevent any media queries using the new syntax from being sorted correctly. For example, this HTML:

    <div class="min-[h:50px]:underline min-[h:200px]:underline min-[h:100px]:underline">
        <!-- ... -->
    </div>
    

    ...generates this CSS, which is in the wrong order:

    @media (min-height: 100px) {
      .min-\[h\:100px\]\:underline {
          text-decoration-line: underline;
      }
    }
    
    @media (min-height: 200px) {
      .min-\[h\:200px\]\:underline {
          text-decoration-line: underline;
      }
    }
    
    @media (min-height: 50px) {
      .min-\[h\:50px\]\:underline {
          text-decoration-line: underline;
      }
    }
    

We've found the only way to successfully stay on top of issues and PRs in a project as big as Tailwind is to be pretty strict about either merging or closing a PR when we review it and not leaving it open, because otherwise things really pile up as often people lose interest in pushing things forward or we simply never reach the point where we're sure it's a good idea to merge something. Hope you understand and know we still value the contribution even if it wasn't merged πŸ‘

If still interested in pushing this forward, feel free to open another PR at some point based on the feedback above and we can reconsider. Thanks!

(expand/collapse my resolution notes)
  • πŸ’­ Re syntax
    • the syntax I used β€” the colon prefix syntax is nothing new to TailwindCSS. We use it in a very similar fashion for arbitrary values for resolving ambiguities, which is also pretty close in practice to what we’re trying to achieve here β€” providing a means to resolve the ambiguity in a media query dimension (which defaults to width using width naturally).
    • alternative syntax considerations β€” I also considered using something more like min-h-[100px] and max-h-[100px], but…
      • that would conflict with the existing min-width, max-width, min-height, and max-height utilities
      • using a different syntax from the currently employed dynamic breakpoint syntax added in v3.2 (i.e. max-* and max-*) could potentially be a breaking change for some users
  • βœ… Re sorting β€” sorting is now FIXED as of commit 265b48, and I've included a test for it

brandonmcconnell avatar May 13 '23 16:05 brandonmcconnell

UPDATE: This has all been taken care of now. The full update is in a newer comment, below.

Fyi I'm actively working on one final commit that will improve the sorting to sort in this order:

max-height β†’ max-width β†’ min-height β†’ min-width

in line with the existing sort order of max-width β†’ min-width

(instead of max/width queries being sorted together using their numeric value)

brandonmcconnell avatar May 13 '23 17:05 brandonmcconnell

This is all patched up and ready for re-review. The sorting now works as expected, and I've included a new test for that.

The new and expected order is (in bulleted order):

  • max-height (descending ↓)
  • max-width (descending ↓)
  • min-height (ascending ↑)
  • min-width (ascending ↑)

Here's one example (collapse/expand)

HTML w/ TailwindCSS utilities (the input)

This rather convoluted class string:

<_ class="min-[3px]:font-bold min-[1px]:font-bold min-[2px]:font-bold min-[h:3px]:font-bold min-[h:1px]:font-bold min-[h:2px]:font-bold min-[w:3px]:font-bold min-[w:1px]:font-bold min-[w:2px]:font-bold max-[3px]:font-bold max-[1px]:font-bold max-[2px]:font-bold max-[h:3px]:font-bold max-[h:1px]:font-bold max-[h:2px]:font-bold max-[w:3px]:font-bold max-[w:1px]:font-bold max-[w:2px]:font-bold" />

Generated CSS (the output)

…outputs this correctly sorted and grouped CSS block:

@media (max-height: 3px) {
  .max-\[h\:3px\]\:font-bold {
    font-weight: 700;
  }
}
@media (max-height: 2px) {
  .max-\[h\:2px\]\:font-bold {
    font-weight: 700;
  }
}
@media (max-height: 1px) {
  .max-\[h\:1px\]\:font-bold {
    font-weight: 700;
  }
}
@media (max-width: 3px) {
  .max-\[3px\]\:font-bold,
  .max-\[w\:3px\]\:font-bold {
    font-weight: 700;
  }
}
@media (max-width: 2px) {
  .max-\[2px\]\:font-bold,
  .max-\[w\:2px\]\:font-bold {
    font-weight: 700;
  }
}
@media (max-width: 1px) {
  .max-\[1px\]\:font-bold,
  .max-\[w\:1px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-height: 1px) {
  .min-\[h\:1px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-height: 2px) {
  .min-\[h\:2px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-height: 3px) {
  .min-\[h\:3px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-width: 1px) {
  .min-\[1px\]\:font-bold,
  .min-\[w\:1px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-width: 2px) {
  .min-\[2px\]\:font-bold,
  .min-\[w\:2px\]\:font-bold {
    font-weight: 700;
  }
}
@media (min-width: 3px) {
  .min-\[3px\]\:font-bold,
  .min-\[w\:3px\]\:font-bold {
    font-weight: 700;
  }
}

brandonmcconnell avatar May 13 '23 21:05 brandonmcconnell

@adamwathan @RobinMalfait I wanted to check in on this PR to prevent it from going stale. This is still a feature that would help me in a number of places in my code across several projects.

In the meantime, I'm accounting for the lack of height media queries with inline variants (e.g. [@media(max-height:50dvh)]), which this PR would resolve (e.g. max-[h:50dvh]).

This PR also accounts for properly sorting media queries, as discussed in PR #11217. All of that is done, tested, and included in this PR.

I also have a related issue open on the container queries plugin repo (https://github.com/tailwindlabs/tailwindcss-container-queries/issues/16) here, which addresses the current syntax differences between that plugin and the latest syntax employed by core Tailwind CSS.

brandonmcconnell avatar Nov 28 '23 17:11 brandonmcconnell

Hey finally getting back to this! I think still going to say no to this one for now mostly because of the API β€” I still like min-h-* and max-h-* a lot better.

You mentioned before this conflicts with the min-height and max-height utilities but it doesn't actually because these are variants. This would be totally fine for example with no ambiguity:

<div class="min-h-[200px]:min-h-[100px]">

I think would likely accept this though with that syntax, but against v4 (the next branch) since there likely won't be a v3.5 at this point πŸ‘ Thanks man!

adamwathan avatar Mar 09 '24 15:03 adamwathan