Merged
Conversation
tcbegley
force-pushed
the
popover-fixes
branch
from
f64e358 to
4aa81ca
Compare
tcbegley
approved these changes
Nov 27, 2021
tcbegley
added a commit
that referenced
this pull request
Nov 28, 2021
* Fix issue related to legacy trigger * Fix support for hide_arrow * Add in support for offset * Improve tests * Bump react-bootstrap version * Format * Add additional popover examples * Change default body behaviour * Add legacy integration test * Improve popover doc examples * Format * Update dependencies * Remove redundant Jest config * Set rootClose to false if legacy trigger removed * Remove rogue comma * Fix popover tests Co-authored-by: tcbegley <tomcbegley@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should fix a number of issues with the popover (and overlay) components as raised in #750 . Specifically:
legacypopover was not working - this issue was in theOverlaycomponent, and was due to a fix for an unknown issue in a test a while ago. To fix this, I am now making use of react-bootstrap'srootCloseproperty of the Overlay whenlegacyis selected. Integration test has been added for this.hide_arrowwas no longer working - this was a frustrating fix as it has meant I've had to recreate the react-bootstrap Popover component in the private folder. Unit test has been added for this.offsetwas no longer working - I have added some detail on the structure thatoffsetshould take, and am now passing this into the popperconfig as required. I tried to add a unit test for this, but jest was not playing ball, and I couldn't get it to tell me the location of the popover. I have included an example in the docs though 🤞In addition, I have added a small feature to the Popover, when
body=True, it will automatically render that text in a popover body to remove the need to always wrap the Popover contents.I have also tried to clarify and tidy up some of the docs and added examples for hide_arrow and offset.
One thing to note is that I don't think having multiple trigger options available works very well. For example, having legacy and hover - when clicking on the target, and hovering over it the popover appears, but disappears as the hover disappearing takes priority.