feat(Props Interfaces): add props interface to Button, AccordionTitle, AccordionContent and refactoring the types definitions #130
Conversation
-added export from the lib index.ts for the NotStrictProps type
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
levithomason and
miroslavstastny
as code owners
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
=========================================
Coverage ? 88.94%
=========================================
Files ? 47
Lines ? 778
Branches ? 112
=========================================
Hits ? 692
Misses ? 84
Partials ? 2
Continue to review full report at Codecov.
|
# Conflicts: # src/components/Accordion/Accordion.tsx # src/lib/index.ts
|
I think that as part of the PR you should also use the props interface in |
|
That's actually a good point. Will add the interfaces in the styles as well. |
|
I added the interfaces for the props in the button as well as avatar styles as those PR was merged already. Please review now. |
| @@ -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.
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.
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.
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.
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 =>- it is unclear for developer that variables parameter can be also consumed
- an attempt to consume variables parameter will result in immediate need to change literal type. This will quickly become annoying with each new case :)
// compile-time error! type literal needs to be fixed as well
root: ({ props: { size }, variables }: { props: IAvatarProps }): ICSSInJSStyle =>- also an important factor that it will be much easier to read the following expression
root: ({ props: { size }}: StyleProviderArg<IAvatarProps>): ICSSInJSStyle =>Please, let me know what do you think
There was a problem hiding this comment.
agreed to resolve this problem later, by means of dedicated PRs set
kuzhelov
added
needs author feedback
src/components/Button/Button.tsx
Outdated
| @@ -1,17 +1,35 @@ | |||
| import * as PropTypes from 'prop-types' | |||
| import * as React from 'react' | |||
|
|
|||
| import { UIComponent, childrenExist, customPropTypes } from '../../lib' | |||
| import { UIComponent, childrenExist, customPropTypes, Extendable } from '../../lib' | |||
There was a problem hiding this comment.
I would expect /lib to be runtime utilities the library needs and /types to hold all our types. That would mean moving Extendable 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 this Extendable util. It seems we should keep all these in one place.
Thoughts?
There was a problem hiding this comment.
I added Extendable in the utils.d.ts file in the types and updated other components accordingly. Thanks for the suggestion!
-updated all components with correct imports -added typings for the AccordionTitle and AccordionContent
mnajdova
changed the title
|
As this PR turns out to solve multiple things then just define the Button prop interface, I renamed it accordiongly |
mnajdova
added
🚀 ready for review
and removed
needs author feedback
This PR introduces props interface for the Button component, as part of #117