Refactor types around useTooltip, useTooltipTrigger & useTooltipTriggerState #1372
Conversation
Andarist
requested review from
dannify,
devongovett and
snowystinger
as code owners
| * anchor element. | ||
| * @default 7 | ||
| */ | ||
| offset?: number, |
There was a problem hiding this comment.
Both offset and crossOffset are only relevant for @react-spectrum/tooltip as its the final UI-centric consumer. Those are also included in PositionProps that SpectrumTooltipTriggerProps extends - so there was no need to add those props anywhere in this PR.
There was a problem hiding this comment.
I think they might have been overridden for the @default values in the docs? @snowystinger would know.
There was a problem hiding this comment.
either way, those shouldn't be included here because this type is not related to anything that needs those 2
There was a problem hiding this comment.
True. Do the overrides need to be moved to the spectrum type then?
There was a problem hiding this comment.
Ok, I get it now - I haven't noticed that those defaults were different for offset. I've re-added that appropriately now.
Andarist
force-pushed
the
cleanup/tooltip-related-props
branch
from
55bc9da to
4239eaf
Compare
Andarist
changed the title
| } | ||
|
|
||
| export interface SpectrumTooltipTriggerProps extends TooltipTriggerProps, PositionProps { | ||
| children: [ReactElement, ReactElement] | ||
| } | ||
|
|
||
| export interface TooltipProps { | ||
| children: ReactNode, |
There was a problem hiding this comment.
children is not considered to be a dom prop and thus it's ignored in the useTooltip's implementation:
Andarist
force-pushed
the
cleanup/tooltip-related-props
branch
from
4239eaf to
214ea52
Compare
| */ | ||
| trigger?: 'focus' | ||
| delay?: number |
There was a problem hiding this comment.
Did you move this to its own type because it's unused in the aria hook? Usually we have 3 types: base stately props, stately + aria props, stately + aria + spectrum. This is diverging a bit from that pattern. Given that the prop is optional, is there any harm in including it as an option to aria too?
There was a problem hiding this comment.
Did you move this to its own type because it's unused in the aria hook?
Most likely - yes.
Given that the prop is optional, is there any harm in including it as an option to aria too?
No great harm but it was hard for me to reason about the structure of those various hooks to see how they are supposed to be interconnected when they have been using types with unrelated properties. THis is the least important part of this PR though so I'm OK with reverting this change - just please confirm that you'd like me to do so.
There was a problem hiding this comment.
For now let's keep the existing pattern of the types building on one another. We can think about whether we want to change that separately.
…e default value documented in JSDoc in PositionProps
This reflects reality better - I've refactored those types so particular types only contain types for properties actually utilized by the consuming utility function.
✅ Pull Request Checklist:
📝 Test Instructions:
yarn check-types