[popover2] fix(ContextMenu2): Tooltip2 and autofocus interactions #4744
Conversation
adidahiya
changed the title
s/Popover2Context/Tooltip2ContextPreviews: documentation | landing | table |
|
I probably need to write some more unit tests testing various nesting scenarios, making sure that multiple contexts don't interact with each other unexpectedly. |
packages/popover2/src/tooltip2.tsx
Outdated
| // it was likely created by a parent ContextMenu2 | ||
| return ( | ||
| <Tooltip2Context.Consumer> | ||
| {([state]) => <Tooltip2Provider initialState={state}>{this.renderPopover}</Tooltip2Provider>} |
There was a problem hiding this comment.
what happens if you have nested tooltips? (acknowledging that it's not the best UX, but not ruling out that its possibility 😬 )
There was a problem hiding this comment.
I don't think it would be a possibility in terms of tooltips nested inside tooltip content, but nesting of tooltip targets could be possible (just like nesting of ContextMenu2 targets, as demonstrated in the ContextMenu2 docs example). I'll add a test for it
There was a problem hiding this comment.
oops yes, i meant targets, thanks for clarifying!
| @@ -129,6 +130,9 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C | |||
| ...restProps | |||
| } = props; | |||
|
|
|||
| // ancestor Tooltip2Context state doesn't affect us since we don't care about parent ContextMenu2s, we only want to | |||
| // force disable parent Tooltip2s in certain cases through dispatching actions | |||
| const [, popover2Dispatch] = React.useContext(Tooltip2Context); | |||
There was a problem hiding this comment.
| const [, popover2Dispatch] = React.useContext(Tooltip2Context); | |
| const [, tooltip2Dispatch] = React.useContext(Tooltip2Context); |
should be renamed here and below...
packages/popover2/src/tooltip2.tsx
Outdated
| // it was likely created by a parent ContextMenu2 | ||
| return ( | ||
| <Tooltip2Context.Consumer> | ||
| {([state]) => <Tooltip2Provider initialState={state}>{this.renderPopover}</Tooltip2Provider>} |
There was a problem hiding this comment.
I don't think it would be a possibility in terms of tooltips nested inside tooltip content, but nesting of tooltip targets could be possible (just like nesting of ContextMenu2 targets, as demonstrated in the ContextMenu2 docs example). I'll add a test for it
so there is actually some brittle code in Popover/Popover2 which force disables direct Tooltip/Tooltip2 children here: blueprint/packages/popover2/src/popover2.tsx Line 352 in b2f3862 the above code breaks when there is even one level of indirection (consider a reusable component which renders a Tooltip around something). In places where I'm upgrading to ContextMenu2 I knew that I would need a more robust solution than this existing Popover > Tooltip pattern, so I opted for the React context approach. I may need to consider doing the same thing for Popover2 <> Tooltip2 interactions... |
address self-review, add unit testsPreviews: documentation | landing | table |
|
I've added tests for some nesting scenarios, and verified these scenarios work in docs-app:
I didn't change the Popover2 > Tooltip2 disable behavior in this PR because I didn't want to introduce scope creep here. Might revisit that in a future PR. |
|
|
||
| export const Tooltip2Provider = ({ children, forceDisable }: Tooltip2ProviderProps) => { | ||
| const [state, dispatch] = React.useReducer(tooltip2Reducer, {}); | ||
| // // if we have a parent context controlling our state, just use its value and don't use the local state |
There was a problem hiding this comment.
remove commented code
remove commented codePreviews: documentation | landing | table |
|
Editor's note: this feature was straightforward enough to implement, but notoriously difficult to test. I think I would have more success / an easier time with React Testing Library. Blueprint's testing infra is due for a refresh :/ |
adidahiya
changed the title
Fixes #4742
Checklist
Changes proposed in this pull request:
An alternative (much more complicated) fix for #4719, which avoids the regression described in #4742.
I was not able to implement #4719 by simply using mouse events when
autoFocus={true}on the ContextMenu2 Popover2. For some reason the React event system seems to propagate mouse enter events to the target after the ContextMenu2 is opened, and that re-opens the tooltip on the shared target.So I decided to try this alternative approach where ContextMenu2 and Tooltip2 communicate with each other via React context.
In the
<ContextMenu2><Tooltip2>...</Tooltip2></ContextMenu2>case, the parent ContextMenu2 creates a Tooltip2Context withforceDisable: true, and the child Tooltip2 reacts to that by consuming the context state.In the
<Tooltip2><ContextMenu2>...</ContextMenu2></Tooltip2>case, the child ContextMenu2 dispatches actions to the parent Tooltip2 context, telling it to force disable itself.Reviewers should focus on:
Is this React context approach overkill?
Screenshot