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.

Sign up for GitHub

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 utility props value type about override the Theme interface #345

Closed
wants to merge 13 commits into from
Closed

fix utility props value type about override the Theme interface #345

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7a430a9
fix ThemeNamespaceValue type
strozw Jan 11, 2022
1dbec76
fix `index.test.ts` type warning
strozw Jan 11, 2022
def25d8
add `@types/react-dom`
strozw Jan 11, 2022
7aaa6d3
update `packages/styled-components` tests
strozw Jan 11, 2022
e0942c5
update `packages/emotion` tests
strozw Jan 11, 2022
ecd00c2
remove unnecessary comment
strozw Jan 12, 2022
2fad6a9
update typescript document for emotion
strozw Jan 12, 2022
038f3bc
Revert "fix ThemeNamespaceValue type"
strozw Jan 17, 2022
57d1abf
update package-lock.json
strozw Jan 17, 2022
ddf9537
Update SystemProps to able to use values not defined in the theme wit…
strozw Jan 17, 2022
09ea4eb
Fixed `ThemeGetter` type to match the implementation of `themeGetter`
strozw Jan 17, 2022
b46291d
Fixed `ColProps` and, `col` props implementation when `false` are pa…
strozw Jan 17, 2022
5375b4c
Fixed `RowProps` and, `row` props implementation when `false` are pa…
strozw Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/system/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ declare type SynthesizedPath<T extends {}> = {
export type ThemeNamespaceValue<
K extends string,
T extends ITheme,
> = SynthesizedPath<T[K]>
> = SynthesizedPath<T[K]> | number | string | false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the fix is correct. ThemeNamespaceValue means a value in the theme namespace, nothing else. However, I see what you want to fix, but I think it is not the right place to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.
Sure, my fix wasn't fit for name.

Read the code again and investigate the appropriate corrections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1st, I reverted prev changes, and changed SystemProp insted of ThemeNamespaeceValue for fix utility props. I think this type name means utility props, but what do you think?

2nd, above changes is not cover same problem about each themeGetter. so I changed ThemeGetter for it.

３rd, the first change on SystemProps found an issue where col and row props didn't support false, so we fixed each one.


export interface ThemeGetter<T = any> {
(value: T, defaultValue?: CSSScalar): (props: Props<Theme>) => CSSScalar
Expand Down