typescript(vx-tooltip): re-write package in TypeScript #504
Conversation
| "@vx/bounds": "0.0.189", | ||
| "classnames": "^2.2.5", | ||
| "prop-types": "^15.5.10" |
There was a problem hiding this comment.
We actually would like to keep this around, @airbnb/config-babel adds babel-plugin-typescript-to-proptypes when used with @airbnb/config-typescript which generates PropTypes for react component types/interfaces, and requires us to keep the prop-types dependency though it's no longer referenced in source code.
| className: PropTypes.string, | ||
| style: PropTypes.object, | ||
| children: PropTypes.any, | ||
| type Props = { |
There was a problem hiding this comment.
could we export these for use by consumers, and (optionally) update to TooltipProps? (there's been back and forth on this vs Props internally but have been trying to use the more specific name variant in @vx)
| updateTooltip: PropTypes.func, | ||
| showTooltip: PropTypes.func, | ||
| hideTooltip: PropTypes.func, | ||
| type Props = { |
There was a problem hiding this comment.
can we export these for use by consumers (especially important since it's an HOC) and also change to WithTooltipProvidedProps so it's clear that they are props provided by the HOC, not those of the BaseComponent which is wrapped?
There was a problem hiding this comment.
Will also need to define a type for WithTooltipState
There was a problem hiding this comment.
the types of the state would be the same as part of the WithTooltipProvidedProps, so I defined the state types by using it. If you think it would be better to have it separately - I'll fix it :)
| hideTooltip?: () => void; | ||
| }; | ||
|
|
||
| type ShowTooltipArgs = { |
There was a problem hiding this comment.
could update to Pick<WithTooltipProvidedProps, 'tooltipLeft' | 'tooltipTop' | 'tooltipData'>
| tooltipData?: Props['tooltipData']; | ||
| }; | ||
|
|
||
| type UpdateTooltipArgs = { |
There was a problem hiding this comment.
could update to Pick<WithTooltipProvidedProps, 'tooltipOpen' | 'tooltipLeft' | 'tooltipTop' | 'tooltipData'>
| tooltipOpen?: Props['tooltipOpen']; | ||
| tooltipLeft?: Props['tooltipLeft']; | ||
| tooltipTop?: Props['tooltipTop']; | ||
| tooltipData?: Props['tooltipData']; | ||
| }; | ||
|
|
||
| export default function withTooltip( |
There was a problem hiding this comment.
I think the props aren't quite right for this. For this to be compatible with any BaseComponent there will need to be a Generic here for the BaseComponents own props. Something like this:
function withTooltip<Props extends object = {}>(
BaseComponent: (React.Component | React.PureComponent | React.FC)<ProvidedProps & Props>,
containerProps: WithTooltipContainerProps = ...,
) {
class WrappedComponent extends React.PureComponent<Props, WithTooltipState> {
...
}
}@milesj does this look right?
| }, | ||
| }, | ||
| ) { | ||
| class WrappedComponent extends React.PureComponent { | ||
| constructor(props) { | ||
| constructor(props: Props) { |
There was a problem hiding this comment.
Re comment above I think this will become BaseProps
| @@ -1,44 +1,40 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import { withBoundingRects } from '@vx/bounds'; | |||
There was a problem hiding this comment.
may need a @ts-ignore here as you mentioned because @vx/bounds currently has no types.
| 'tooltipOpen' | 'tooltipLeft' | 'tooltipTop' | 'tooltipData' | ||
| >; | ||
| type ShowTooltipArgs = Pick<WithTooltipProvidedProps, 'tooltipLeft' | 'tooltipTop' | 'tooltipData'>; | ||
| type UpdateTooltipArgs = Pick< |
There was a problem hiding this comment.
same type?
type UpdateTooltipArgs = WithTooltipState;| WithTooltipProvidedProps, | ||
| 'tooltipOpen' | 'tooltipLeft' | 'tooltipTop' | 'tooltipData' | ||
| >; | ||
| type ShowTooltipArgs = Pick<WithTooltipProvidedProps, 'tooltipLeft' | 'tooltipTop' | 'tooltipData'>; |
There was a problem hiding this comment.
type ShowTooltipArgs = Omit<WithTooltipState, 'tooltipOpen'>;
| export default function withTooltip( | ||
| BaseComponent, | ||
| containerProps = { | ||
| export default function withTooltip<Props extends object = {}>( |
There was a problem hiding this comment.
looks good, if the user specifies props on their component as OwnProps & WithTooltipProvidedProps, this should have everything on it 👍
| style, | ||
| children, | ||
| ...restProps | ||
| }: TooltipProps) { |
There was a problem hiding this comment.
missed this one earlier, but we'd like to preserve the ability to pass additional restProps here so we don't make breaking changes. the pattern I think we've aligned on is updating this line to TooltipProps & JSX.IntrinsicElements.div so that TooltipProps can still be imported separately but the component accepts any div props as well.
There was a problem hiding this comment.
oh, you are right! It's a great point, thanks!
| height: 'inherit', | ||
| position: 'relative' as const, | ||
| width: 'inherit' as const, | ||
| height: 'inherit' as const, | ||
| }, |
There was a problem hiding this comment.
could do
style: {
position: 'relative',
width: 'inherit',
height: 'inherit'
} as const
|
@Rudeg FYI I updated the base branch to |
🚀 Enhancements
This PR builds off of #488 which added TypeScript build support, and converts the @vx-tooltip package to TypeScript.
tooltipcomponent depends onvx/bounds, so CI might be failing because of this. Closes #505.Testing
@williaster @hshoff @schillerk @milesj @kristw