Fix some prop warnings #5635
Fix some prop warnings #5635
Conversation
tmeasday
requested review from
Hypnosphi,
igor-dv,
ndelangen and
theinterned
as code owners
There was a problem hiding this comment.
Looking for some answers from @ndelangen and @domyen before merging this.
| @@ -8,6 +8,7 @@ import deprecate from 'util-deprecate'; | |||
|
|
|||
| export interface RenderOptions { | |||
| active: boolean; | |||
| key: string; | |||
There was a problem hiding this comment.
@ndelangen can you confirm this is correct?
| if (onVisibilityChange !== undefined) { | ||
| Object.assign(props, { onVisibilityChange }); | ||
| } | ||
|
|
There was a problem hiding this comment.
@ndelangen I don't understand what the purpose of this technique was
| return ( | ||
| <TooltipTrigger | ||
| placement={placement} | ||
| trigger={trigger} | ||
| modifiers={modifiers} | ||
| {...props} |
There was a problem hiding this comment.
We shouldn't pass props both too the TooltipTrigger and the Container (below). It should be one of the other. @domyen which one makes sense to you?
There was a problem hiding this comment.
The only thing I use ...props for is styled-components. Alternatively, if we're not passing props here it might make more sense to explicitly use className
There was a problem hiding this comment.
@domyen my point was previously this component was doing that in two points (the TooltipTrigger and the Container). It seems wrong (and it caused problems) to do that
|
@tmeasday please update snapshots |
|
@shilman merged |
shilman
added
the
patch:done
Issue: #5600
What I did
Fix up some warnings from the logs
How to test
Run official storybook