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

Refactor some screen compatibility code #14438

Closed
wants to merge 3 commits into from

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Sep 16, 2024

This adds support for the screen(…) CSS function which can be used to build the media queries for a given breakpoint:

Input:

@theme {
  --breakpoint-sm: 640px;
}

@media screen(sm) {
  .foo {
    color: red;
  }
}

output:

@media (width >= 640px) {
  .foo {
    color: red;
  }
}

We also support more advanced screens configurations from configuration files (mainly for backwards) compat:

export default {
  theme: {
    screens: {
      sm: '640px',
      md: { min: '768px' },
      lg: { min: '1024px', max: '1280px' },
      sidebar: [
        { min: '640px', max: '768px' },
        { min: '1024px' },
      ],
    }
  }
}
@media screen(sm) {
  .foo {
    color: red;
  }
}

@media screen(md) {
  .foo {
    color: blue;
  }
}

@media screen(lg) {
  .foo {
    color: green;
  }
}

@media screen(sidebar) {
  .foo {
    color: yellow;
  }
}

Output:

@media (width >= 640px) {
  .foo {
    color: red;
  }
}

@media (width >= 768px) {
  .foo {
    color: blue;
  }
}

@media (1280px >= width >= 1024px) {
  .foo {
    color: green;
  }
}

@media (768px >= width >= 640px), (width >= 1024px) {
  .foo {
    color: yellow;
  }
}

@thecrypticace
Copy link
Contributor Author

@philipp-spiess [@media(screen(sm))]:flex doesn't work however [@media_screen(sm)]:flex does work — presumably because of the space. We should take a look at the value parser.

@philipp-spiess
Copy link
Member

@philipp-spiess [@media(screen(sm))]:flex doesn't work however [@media_screen(sm)]:flex does work — presumably because of the space. We should take a look at the value parser.

@thecrypticace Pushed a commit that fixes that :) The value parser is actually not the issue here, it's that we were only looking into @screen if it starts with @screen

@thecrypticace thecrypticace force-pushed the fix/v4-no-throw-on-theme-in-candidate branch from b4e83e8 to ea69d99 Compare September 17, 2024 16:33
Base automatically changed from fix/v4-no-throw-on-theme-in-candidate to next September 17, 2024 16:46
This will ensure that a breakpoint defined with an object like `lg: { min: "1280px" }` will also support `max-lg:*`
@thecrypticace thecrypticace marked this pull request as ready for review September 18, 2024 20:08
@thecrypticace thecrypticace changed the title Implement screen() in CSS Add support for the screen() function in CSS Sep 18, 2024
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still not great that we now have the screen function checks in core but this way the legacy implementation is at least contained.

Just brainstorming here, but one way to clean this up even further is by adding another walk for function replacement in the compat layer and we'd need a way for the compat layer to hook into the build calls (since we do a function substitution there first).

Other idea is that we do the function replacement after the core layer and have a dictionary of functions that we can register (just like our Utilities and Variants maps). Then, the compat layer could push custom functions into it.

Comment on lines 194 to 199
// Replace `resolveBreakpointQuery` with a version that can handle more
// complex breakpoint queries like objects, tuples, and custom media queries
// that are present in JS theme configurations.
designSystem.resolveBreakpointQuery = function resolveBreakpointQuery(name: string) {
return buildMediaQuery(pluginApi.theme(`screens.${name}`))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

@thecrypticace thecrypticace changed the title Add support for the screen() function in CSS Refactor some screen compatibility code Sep 19, 2024
@thecrypticace
Copy link
Contributor Author

We've decided to do this as a code mod instead — I'll be landing some of the refactoring tweaks in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants