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

Allow nested selectors #13

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Allow nested selectors #13

merged 1 commit into from
Aug 18, 2021

Conversation

jsakas
Copy link
Contributor

@jsakas jsakas commented Aug 18, 2021

@garronej this seems to work, by calling emotion css function twice - but it breaks the types.

Screen Shot 2021-08-18 at 1 46 04 PM

@jsakas
Copy link
Contributor Author

jsakas commented Aug 18, 2021

Actually if I change:

            getCssObjectOrCssObject:
                | ((
                      theme: Theme,
                      params: Params,
                      classes: Record<Key, string>,    // <-- here
                  ) => Record<Key, CSSObject>)
                | Record<Key, CSSObject>,

to:

            getCssObjectOrCssObject:
                | ((
                      theme: Theme,
                      params: Params,
                      classes: any,
                  ) => Record<Key, CSSObject>)
                | Record<Key, CSSObject>,

But I am not totally sure why that happens. Also any seems like not the right choice here but it does get it working.

@garronej
Copy link
Owner

Damn,
I though it was the runtime that would be hard to get working but it's actually the typings that turn out to be a problem.
I got the classes parameter to be inferred as Record<string, string> but I am still failing to to get it as Record<"el1" | "el2", string>...
Good job on the implementation though.

@garronej
Copy link
Owner

I am very disappointed 😞
I think it's a no go with the current state of typescript.
Trying to think of another API...

@jsakas
Copy link
Contributor Author

jsakas commented Aug 18, 2021

I am very disappointed 😞
I think it's a no go with the current state of typescript.
Trying to think of another API...

Calling css function twice isn't really ideal either. I'm not sure if there is a performance hit, but it seems hacky.

I am going to run this in the meantime, until a better API emerges.

@jsakas
Copy link
Contributor Author

jsakas commented Aug 18, 2021

@garronej also, I pushed up the change from any to Record<string, string> since it's at least a slight improvement. Even without the typings on the classes that come back in useStyles the work might still be useful to some 🤷🏻‍♂️

You might still consider a merge and then a minor version bump later in the case of a better API.

@jsakas jsakas marked this pull request as ready for review August 18, 2021 20:31
@garronej
Copy link
Owner

Hold on a minute, I want to know what you think about this

@garronej
Copy link
Owner

What do you think of that:

image

It's less sexy but also less voodooish and type safe...

@jsakas
Copy link
Contributor Author

jsakas commented Aug 18, 2021

Seems fine to me if it gets the job done!

@garronej
Copy link
Owner

I test it and release it in the next hour.
Thank you very much for your involvement, it helps a lot

@garronej
Copy link
Owner

I will merge even if I override it. This way you'll appear in the contributor list.

@garronej garronej merged commit c15f6eb into garronej:main Aug 18, 2021
gitbook-com bot pushed a commit that referenced this pull request Jan 22, 2022
gitbook-com bot pushed a commit that referenced this pull request Jul 10, 2022
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