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 support for dimension prefix when using dynamic min-* and max-* #11217

Conversation

brandonmcconnell
Copy link
Contributor

@brandonmcconnell brandonmcconnell commented May 11, 2023

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.

@RobinMalfait RobinMalfait force-pushed the support-arbitrary-min-max-direction-prefix branch from 92354e1 to b7ea2cb Compare May 12, 2023 17:06
@RobinMalfait
Copy link
Contributor

Rebased your PR on master such that the commits are clean. Also make sure to pull in the latest master branch so that your local copy is up to date as well 👍

@adamwathan
Copy link
Member

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!

@adamwathan adamwathan closed this May 13, 2023
@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented May 13, 2023

@adamwathan Thanks for the feedback! A few 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 — I thought I had accounted for sorting already, but I guess not. I’ll patch that up and make sure to get the sorting implementation right this time. It shouldn’t take much additional effort to account for this change in the sorting piece.

I’ll also include these follow-up notes in the new PR when I open it, for convenience. Thanks again!

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

3 participants