Fixes issue with outside clicks on a secondary popover trigger (v2) #1233
Conversation
| @@ -292,41 +292,52 @@ exports[`EuiInMemoryTable behavior pagination 1`] = ` | |||
| panelPaddingSize="none" | |||
There was a problem hiding this comment.
This diff looks a lot worse than it is (VS Code was able to actually show the diff much better for some reason). The only change here is that the wrapping outside click detector component is no longer present because the popover uses isOpen={false} here.
| @@ -35,7 +35,7 @@ export default class extends Component { | |||
| iconSide="right" | |||
| onClick={this.onButtonClick.bind(this)} | |||
| > | |||
| Show popover | |||
| Toggle popover | |||
There was a problem hiding this comment.
Changed this to emphasize that this example demonstrates a button that works as a toggle, whereas the secondary click example demonstrates a button that only opens (which I hope will help explain that the secondary trigger cannot be set up to do that, it will always work as a toggle). I could be overthinking whether this is really worth explaining in the docs. :)
|
I was a bit surprised that this change didn't cause any of the popover tests to fail. I think it might be because those tests are using render and mount instead of shallow, so the outside click detector disappears from the output either way? But I didn't want to mess with those tests too much. Let me know if you have thoughts there. UPDATE: Added 2 tests for presence of outside click detector component. Also unified the "id" prop to be a hard-coded single value so that snapshots don't fail when new tests are added in that popover test file. |
| @@ -3,7 +3,7 @@ | |||
| exports[`EuiPopover children is rendered 1`] = ` | |||
There was a problem hiding this comment.
This file was using a getId() function to generate sequential id values for each test. That made a bunch of snapshots "fail" when new tests were added in between others, which didn't seem useful. I talked to @chandlerprall and we agreed hard-coding these ids is better (not sure if the IDs really even need to exist in the test tbh, but left them there as hard-coded values for now).
| .toMatchSnapshot(); | ||
| }); | ||
|
|
||
| test('outside click detector is not rendered when closed', () => { |
There was a problem hiding this comment.
The diff is a little mixed up, but this is 1 of the 2 new tests.
| id="popover" | ||
| button={button} | ||
| isOpen={this.state.isPopoverOpen} | ||
| closePopover={this.closePopover} |
There was a problem hiding this comment.
The closePopover doesn't seem to work
| Usually you pass a button node to the popover whose onClick handler | ||
| will open the popover or toggle the popover open and closed. You can | ||
| also trigger this behavior from other elements that live outside of | ||
| the popover element. Note: clicks on these outside elements while |
There was a problem hiding this comment.
Let's move the note into a warning callout.
|
Is it possible that #1527 (now part of |
|
@jasonrhodes Mind responding to Greg's comment above? Doing some repo cleanup and closing out old PRs. This one might not need to be around anymore. |
|
Yeah that looks like it probably solves this. Don't have time to fully test right now but I'll re-open if I discover differently later -- thanks! |
Summary
Closes #1218 -- only renders the "outside click detector" component when the popover is open, so that the listeners are mounted and unmounted at the correct times and you don't get unexpected listeners firing.
Note: this is "v2" of this PR because the first one solved this same problem in a more complicated way which led to many more snapshot updates, and the PR also included a ton of editor-driven prettier formatting changes that weren't relevant for this repo.
Checklist
Any props added have proper autodocsThis was checked for breaking changes and labeled appropriately