-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Updated types to support global Theme definition #1609
Changes from 13 commits
07a70a4
3ad510f
c55d448
7be6f64
353dc2e
358c12d
23717d4
36ea5ee
1c0d7c3
84bab57
44adafb
2e8b12b
bef05bd
2025fa2
f9ff2b2
4649226
94d0d1f
c2fb7f1
f391e6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,9 @@ export { | |
export * from './theming' | ||
export * from './helper' | ||
|
||
// tslint:disable-next-line: no-empty-interface | ||
export interface Theme {} | ||
|
||
export const ThemeContext: Context<object> | ||
export const CacheProvider: Provider<EmotionCache> | ||
export function withEmotionCache<Props, RefType = any>( | ||
|
@@ -57,16 +60,15 @@ export type InterpolationWithTheme<Theme> = | |
| Interpolation | ||
| ((theme: Theme) => Interpolation) | ||
|
||
export interface GlobalProps<Theme> { | ||
styles: InterpolationWithTheme<Theme> | ||
export interface GlobalProps { | ||
styles: Interpolation<Theme> | ||
} | ||
|
||
/** | ||
* @desc | ||
* JSX generic are supported only after [email protected] | ||
*/ | ||
export function Global<Theme extends {} = any>( | ||
props: GlobalProps<Theme> | ||
): ReactElement | ||
export function Global(props: GlobalProps): ReactElement | ||
|
||
export function keyframes( | ||
template: TemplateStringsArray, | ||
|
@@ -83,26 +85,24 @@ export type ClassNamesArg = | |
| { [className: string]: boolean | null | undefined } | ||
| ArrayClassNamesArg | ||
|
||
export interface ClassNamesContent<Theme> { | ||
export interface ClassNamesContent { | ||
css(template: TemplateStringsArray, ...args: Array<Interpolation>): string | ||
css(...args: Array<Interpolation>): string | ||
cx(...args: Array<ClassNamesArg>): string | ||
theme: Theme | ||
} | ||
export interface ClassNamesProps<Theme> { | ||
children(content: ClassNamesContent<Theme>): ReactNode | ||
export interface ClassNamesProps { | ||
children(content: ClassNamesContent): ReactNode | ||
} | ||
/** | ||
* @desc | ||
* JSX generic are supported only after [email protected] | ||
*/ | ||
export function ClassNames<Theme extends {} = any>( | ||
props: ClassNamesProps<Theme> | ||
): ReactElement | ||
export function ClassNames(props: ClassNamesProps): ReactElement | ||
|
||
declare module 'react' { | ||
interface DOMAttributes<T> { | ||
css?: InterpolationWithTheme<any> | ||
css?: Interpolation | ||
} | ||
} | ||
|
||
|
@@ -114,7 +114,7 @@ declare global { | |
*/ | ||
|
||
interface IntrinsicAttributes { | ||
css?: InterpolationWithTheme<any> | ||
css?: Interpolation | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
// Definitions by: Junyoung Clare Jang <https://github.com/Ailrun> | ||
// TypeScript Version: 2.8 | ||
|
||
import { Theme } from '@emotion/core' | ||
import { RegisteredCache, SerializedStyles } from '@emotion/utils' | ||
import * as CSS from 'csstype' | ||
|
||
|
@@ -15,8 +16,10 @@ export type CSSPropertiesWithMultiValues = { | |
/** | ||
* @desc Following type exists for autocompletion of key. | ||
*/ | ||
export type CSSPseudos<MP> = { [K in CSS.Pseudos]?: ObjectInterpolation<MP> } | ||
export interface CSSOthersObject<MP> { | ||
export type CSSPseudos<MP = { theme: Theme }> = { | ||
[K in CSS.Pseudos]?: ObjectInterpolation<MP> | ||
} | ||
export interface CSSOthersObject<MP = { theme: Theme }> { | ||
[propertiesName: string]: Interpolation<MP> | ||
} | ||
|
||
|
@@ -56,17 +59,18 @@ export type Keyframes = { | |
toString: () => string | ||
} & string | ||
|
||
export interface ArrayInterpolation<MP> extends Array<Interpolation<MP>> {} | ||
export interface ObjectInterpolation<MP> | ||
export interface ArrayInterpolation<MP = { theme: Theme }> | ||
extends Array<Interpolation<MP>> {} | ||
export interface ObjectInterpolation<MP = { theme: Theme }> | ||
extends CSSPropertiesWithMultiValues, | ||
CSSPseudos<MP>, | ||
CSSOthersObject<MP> {} | ||
|
||
export interface FunctionInterpolation<MergedProps> { | ||
export interface FunctionInterpolation<MergedProps = { theme: Theme }> { | ||
(mergedProps: MergedProps): Interpolation<MergedProps> | ||
} | ||
|
||
export type Interpolation<MergedProps = undefined> = | ||
export type Interpolation<MergedProps = { theme: Theme }> = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this and the I also invited you @JakeGinnivan as a member to my repo, so it's not mandatory to have to PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kinda backed away from these types during my initial work. I wonder if we should just not have a default? (then if not specified it will be unknown) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just for my understanding, why is this the case? Looking at the other types elsewhere we could actually do another round of simplification if we could add theme into these types directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've meant A different story is the
What kind of simplification do you have in mind? I've noticed now that we have Interpolation, CSSInterpolation and even InterpolationWithTheme and I think they might be used sometimes in wrong contexts (especially the Interpolation type). I need to draw out the dependency tree of those types somewhere for myself to analyze this further 😅 |
||
| null | ||
| undefined | ||
| boolean | ||
|
@@ -79,7 +83,7 @@ export type Interpolation<MergedProps = undefined> = | |
| ObjectInterpolation<MergedProps> | ||
| FunctionInterpolation<MergedProps> | ||
|
||
export function serializeStyles<MP>( | ||
export function serializeStyles<MP = { theme: Theme }>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - |
||
args: Array<TemplateStringsArray | Interpolation<MP>>, | ||
registered: RegisteredCache, | ||
mergedProps?: MP | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole docs on Theme usage with TS feels massively outdated now, and I feel like it should be reworded altogether.
Exposing for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - I feel like we can handle it in a separate PR though. Ideally, such docs would present the current solution with its tradeoffs and incorporate stuff from this #973
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not in this PR, it should definitely be a blocker for releasing v11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is crucial and I plan to get this done before v11 👍