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

Support dynamic value #3146

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

siriwatknp
Copy link

What:

Add callback support for the value when serializing styles. For example:

styled('div')({
  background: (props) => props.color || '#fff',
  '&:hover': {
    background: (props) => props.color || 'red',
  }
})

Why:

  1. We (MUI) need emotion to have a consistent API with our new static CSS-in-JS library to let developers switch between engines.
  2. styled-components already support the callback for the values so it's not new.

How:

The change is mainly in emotion's serialize function. When the value is a function, just call it with the props and continue serializing the result the same as before.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

Copy link

changeset-bot bot commented Jan 8, 2024

⚠️ No Changeset found

Latest commit: 2fb22d4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codesandbox-ci bot commented Jan 8, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2fb22d4:

Sandbox Source
Emotion Configuration

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b0014b4) 95.07% compared to head (2fb22d4) 95.08%.

Additional details and impacted files
Files Coverage Δ
packages/serialize/src/index.js 100.00% <100.00%> (ø)


export type CSSInterpolation = InterpolationPrimitive | ArrayCSSInterpolation
export type CSSInterpolation<Props = unknown> =
Copy link
Member

Choose a reason for hiding this comment

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

This one actually shouldn't end up including functions in CSSObject. We have 2 flavors of those interpolations:

  • the ones that u can pass to css
  • and the ones that you can pass to styled and similar

The difference is that css is a context-less call. We don't get access to any props there (and we don't defer css resolution) so this should be an error:

css({
  display: () => 'flex'
})

That's why we have both ArrayInterpolation and ArrayCSSInterpolation here. A similar distinction should be introduced for those object styles.

But now... I'm not sure if this would satisfy your original goal.

Copy link
Author

Choose a reason for hiding this comment

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

I see, I will revert this. I think it should be fine since we focus only for styled.

Copy link
Author

@siriwatknp siriwatknp Jan 9, 2024

Choose a reason for hiding this comment

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

Done! see 2fb22d4

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be fine since we focus only for styled.

Cool! ❤️

Done! see 2fb22d4

Thanks. I'm soft-approving this PR now. I think we still need to tweak types a little bit but honestly - I need to give it more thought to determine how to do it in the least disruptive way. So I think, it's best for me to do it soon-ish since I don't have concrete advice right now on how this should be done.

I'll also give @emmatown a couple of days to give their opinion about this. If we don't merge it this week - please ping me later.

@siriwatknp siriwatknp requested a review from Andarist January 9, 2024 04:54
@siriwatknp
Copy link
Author

@Andarist Would you mind taking a look at this again?

@Andarist
Copy link
Member

I think we can consider this to be approved but I still have to figure out how types should be structured and I have some other things on my plate right now (including figuring out how to integrate Emotion with the upcoming React 19). How urgent/blocking this one is for you? it might help me to prioritize this PR.

@siriwatknp
Copy link
Author

I think we can consider this to be approved but I still have to figure out how types should be structured and I have some other things on my plate right now (including figuring out how to integrate Emotion with the upcoming React 19). How urgent/blocking this one is for you? it might help me to prioritize this PR.

I'd say the urgency is medium. We are in the process of migrating components to support both emotion and zero-runtime engine, some need dynamic callback support so those are blocked by this PR.

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.

2 participants