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 typing for create-emotion-styled #671

Merged
merged 5 commits into from
May 25, 2018

Conversation

Ailrun
Copy link
Member

@Ailrun Ailrun commented May 24, 2018

What:

  1. All types introduced in Add typing for create-emotion-styled #668 live in single file, so it's hard to write preact type using common things.
    I do really hate global JSX namespace idea.
  2. It used ReactHTML, but it's not an interface for props, but for factories. Therefore it does not provide HTML attributes like onClick as props.
  3. ReactHTML problem occurs because of lack of tests, so I add some tests too.

Why: Typing introduced in #668 is wrong.

How: By using JSX.IntrinsicElements

Checklist:

  • [N/A] Documentation
  • Tests
  • Code complete

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

LGTM

One thing I forgot about last time is the shorthand, while it only works with the babel plugin, there are many people who run ts and then babel with babel-plugin-emotion so could you make this work?

styled.div(...styles)

@Ailrun
Copy link
Member Author

Ailrun commented May 25, 2018

@mitchellhamilton
Oh, I also forgot about to ask it. Does babel plugin work with create-emotion-styled too?

@emmatown
Copy link
Member

@Ailrun yes, the babel plugin does work with create-emotion-styled.

@Ailrun Ailrun force-pushed the typing/create-emotion-styled branch from a540650 to 1915489 Compare May 25, 2018 23:06
[T in keyof JSX.IntrinsicElements]: CreateStyledOtherComponent<JSX.IntrinsicElements[T], Theme>;
};

export interface CreateStyled<Theme extends object = any>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this needs Theme extends object = any whereas the shorthand one doesn't need = any?

Copy link
Member Author

@Ailrun Ailrun May 25, 2018

Choose a reason for hiding this comment

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

@mitchellhamilton CreateStyled is access point of all API, where CreateStyledShorthands is just a base interface (well, it's not really interface, but in some way, it's similar to interface) of CreateStyled.
Therefore, CreateStyled is one used by user, so I put the default type on this interface only.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

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