-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Props Interfaces): add props interface to Button, AccordionTitle, AccordionContent and refactoring the types definitions #130
Changes from 7 commits
f0c7f78
7e4ccb1
ce08f73
f81dac4
51f7afd
da3c3cb
28315e4
86f579f
a155684
dbc2cb6
62c6fb2
4c4cf61
0d804db
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 |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { pxToRem } from '../../../../lib' | ||
import { IComponentPartStylesInput, ICSSInJSStyle } from '../../../../../types/theme' | ||
import { IAvatarProps } from '../../../../components/Avatar/Avatar' | ||
|
||
const getAvatarDimension = (size: number) => { | ||
return 12 + size * 4 | ||
|
@@ -49,7 +50,7 @@ const getAvatarFontSize = (size: number) => { | |
} | ||
|
||
const avatarStyles: IComponentPartStylesInput = { | ||
root: ({ props: { size } }): ICSSInJSStyle => ({ | ||
root: ({ props: { size } }: { props: IAvatarProps }): ICSSInJSStyle => ({ | ||
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. seems that it is better for us to introduce general type for that as well, instead of inline type literal. Let me take a closer look to propose a suggestion 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. Well generally the the object that we are receiving here is {props, variables} so we may introduce this, but the thing is this is not used everywhere, in some cases we have just the props, or nothing which isn't a problem, but when we have just the pros or nothing we immediately know that this part of the styles is somewhat static and doesn't depend on any prop of variable. So I am not sure if we wan to specify specific types which may won't be used... 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. Should we not wait until we figure out the optimal typing pattern for styles and variables before typing them? Especially as we plan to change this signature. We haven't yet figured out how 3rd parties will create themes, but we know they will need access to component typings as well so that they can type their styles/variables as well. Not sure what the best way to type these is but I know from testing it myself that there are several ways to go about it, each with their own pros and cons. I would propose that PRs opened against #117 for the purpose of typing props do not also attempt to type themes, just props for now. 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. lets try to follow the thought to see if we are on the same page. Seems that, at least semantically, the type of functions that are used for styling is the following (names are made up just as an example): type StyleProvider<TProps, TVariables = any> = (args: StyleProviderArg) => ICSSInJSStyle`
type StyleProviderArg<TProps, TVariables = any> = { props: TProps, variables: TVariables } To me it seems reasonable to declare this knowledge explicitly - because otherwise we would have the following issues for the following form: root: ({ props: { size } }: { props: IAvatarProps }): ICSSInJSStyle =>
// compile-time error! type literal needs to be fixed as well
root: ({ props: { size }, variables }: { props: IAvatarProps }): ICSSInJSStyle =>
root: ({ props: { size }}: StyleProviderArg<IAvatarProps>): ICSSInJSStyle => Please, let me know what do you think 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. agreed to resolve this problem later, by means of dedicated PRs set |
||
backgroundColor: 'inherit', | ||
display: 'inline-block', | ||
verticalAlign: 'top', | ||
|
@@ -59,7 +60,7 @@ const avatarStyles: IComponentPartStylesInput = { | |
imageAvatar: (): ICSSInJSStyle => ({ | ||
verticalAlign: 'top', | ||
}), | ||
avatarNameContainer: ({ props: { size } }): ICSSInJSStyle => ({ | ||
avatarNameContainer: ({ props: { size } }: { props: IAvatarProps }): ICSSInJSStyle => ({ | ||
display: 'inline-block', | ||
width: pxToRem(getAvatarDimension(size)), | ||
height: pxToRem(getAvatarDimension(size)), | ||
|
@@ -68,7 +69,13 @@ const avatarStyles: IComponentPartStylesInput = { | |
verticalAlign: 'top', | ||
textAlign: 'center', | ||
}), | ||
presenceIndicatorWrapper: ({ props: { size }, variables: v }): ICSSInJSStyle => ({ | ||
presenceIndicatorWrapper: ({ | ||
props: { size }, | ||
variables: v, | ||
}: { | ||
props: IAvatarProps | ||
variables: any | ||
}): ICSSInJSStyle => ({ | ||
position: 'relative', | ||
top: `-${pxToRem(getPresenceIndicatorWrapperTop(size))}`, | ||
left: pxToRem(getPresenceIndicatorWrapperLeft(size)), | ||
|
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.
I would expect
/lib
to be runtime utilities the library needs and/types
to hold all our types. That would mean movingExtendable
to/types
as well.There are a few more typing utilities in
/types/theme.d.ts
(ObjectOf, OneOrArray) that we could put in a separate/types/utils
along with thisExtendable
util. It seems we should keep all these in one place.Thoughts?
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.
totally agree 👍
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.
I added Extendable in the utils.d.ts file in the types and updated other components accordingly. Thanks for the suggestion!