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

fix(preset-mini): support composite props #3810

Merged
merged 12 commits into from
Jun 11, 2024
Merged

fix(preset-mini): support composite props #3810

merged 12 commits into from
Jun 11, 2024

Conversation

zyyv
Copy link
Member

@zyyv zyyv commented May 16, 2024

close #3807.

@zyyv zyyv requested a review from antfu as a code owner May 16, 2024 02:33
Copy link

netlify bot commented May 16, 2024

Deploy Preview for unocss ready!

Name Link
🔨 Latest commit 9c5976f
🔍 Latest deploy log https://app.netlify.com/sites/unocss/deploys/666868ab9484db0008266092
😎 Deploy Preview https://deploy-preview-3810--unocss.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Curstantine
Copy link

Curstantine commented May 16, 2024

Wouldn't this break anything that depended on h.properties?
For an example, transition-height and etc would completely break.

This is why the `targets' test is failing.

image
image

@Curstantine
Copy link

Curstantine commented May 16, 2024

Since this PR adds a composite prop to the CSS props array in handler utils, it more or less breaks the whole prop resolving mechanism.

I think the better approach is to remove the composite prop from that handler array, and guard the h.properties value like the codebase has done before. Moreover, since h.properties only returns valid CSS values, we don't have to transform it further.

So an approach like this passes the test cases. Though for some odd reason the snapshot matching fails.

  1. Early return when properties are valid CSS properties.
  2. Transform the "custom" properties, filter undefined ones, and only return a valid string when the array itself is populated.
function transitionProperty(prop: string): string | undefined {
  const cssProps = h.properties(prop)
  if (cssProps !== undefined)
    return cssProps

  const transformed = prop.split(',').flatMap(x => transitionPropertyGroup[x]).filter(x => x !== undefined)
  return transformed.length > 0 ? transformed.join(',') : undefined
}

image

Curstantine added a commit to Curstantine/rxrcd that referenced this pull request May 16, 2024
@antfu antfu enabled auto-merge June 11, 2024 15:09
@antfu antfu added this pull request to the merge queue Jun 11, 2024
Merged via the queue into unocss:main with commit f7ad520 Jun 11, 2024
9 checks passed
@zyyv zyyv deleted the issue-3807 branch June 12, 2024 01:59
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.

Properties with multiple values are not matched against shortcuts.
3 participants