Skip to content

Commit

Permalink
feat(Popover): add legacy trigger, replacing isInteractive prop
Browse files Browse the repository at this point in the history
  • Loading branch information
@TheSharpieOne
TheSharpieOne committed Jan 15, 2019
1 parent 1d5ce83 commit 6b3c3ce
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 29 deletions.
1 change: 1 addition & 0 deletions docs/lib/Components/PopoversPage.js
View file
Expand Up @@ -82,6 +82,7 @@ export default class PopoversPage extends React.Component {
</PrismCode>
</pre>
<SectionTitle>Popovers Trigger</SectionTitle>
<p>Trigger each popover to see information about the trigger</p>
<div className="docs-example">
<PopoverFocusExample />
</div>
Expand Down
44 changes: 23 additions & 21 deletions docs/lib/examples/PopoverFocus.js
View file
@@ -1,33 +1,35 @@
/* eslint react/no-multi-comp: 0, react/prop-types: 0 */
import React from 'react';
import { Button, Popover, PopoverHeader, PopoverBody } from 'reactstrap';
import { Button, UncontrolledPopover, PopoverHeader, PopoverBody } from 'reactstrap';

export default class Example extends React.Component {
constructor(props) {
super(props);

this.toggle = this.toggle.bind(this);
this.state = {
popoverOpen: false
};
}

toggle() {
this.setState({
popoverOpen: !this.state.popoverOpen
});
}

render() {
return (
<div>
<Button id="PopoverFocus" type="button">
Launch Popover
Launch Popover (Focus)
</Button>
{' '}
<Button id="PopoverClick" type="button">
Launch Popover (Click)
</Button>
{' '}
<Button id="PopoverLegacy" type="button">
Launch Popover (Legacy)
</Button>
<Popover trigger="focus" placement="bottom" isOpen={this.state.popoverOpen} target="PopoverFocus" toggle={this.toggle}>
<PopoverHeader>Popover Title</PopoverHeader>
<PopoverBody>Sed posuere consectetur est at lobortis. Aenean eu leo quam. Pellentesque ornare sem lacinia quam venenatis vestibulum.</PopoverBody>
</Popover>
<UncontrolledPopover trigger="focus" placement="bottom" target="PopoverFocus">
<PopoverHeader>Focus Trigger</PopoverHeader>
<PopoverBody>Focusing on the trigging element makes this popover appear. Blurring (clicking away) makes it disappear. You cannot select this text as the popover will disappear when you try.</PopoverBody>
</UncontrolledPopover>
<UncontrolledPopover trigger="click" placement="bottom" target="PopoverClick">
<PopoverHeader>Click Trigger</PopoverHeader>
<PopoverBody>Clicking on the triggering element makes this popover appear. Clicking on it again will make it disappear. You can select this text, but clicking away (somewhere other than the triggering element) will not dismiss this popover.</PopoverBody>
</UncontrolledPopover>
<UncontrolledPopover trigger="legacy" placement="bottom" target="PopoverLegacy">
<PopoverHeader>Legacy Trigger</PopoverHeader>
<PopoverBody>
Legacy is a reactstrap special trigger value (outside of bootstrap's spec/standard). Before reactstrap correctly supported click and focus, it had a hybrid which was very useful and has been brought back as trigger="legacy". One advantage of the legacy trigger is that it allows the popover text to be selected while also closing when clicking outside the triggering element and popover itself.</PopoverBody>
</UncontrolledPopover>
</div>
);
}
Expand Down
16 changes: 8 additions & 8 deletions src/TooltipPopoverWrapper.js
View file
Expand Up @@ -37,7 +37,6 @@ export const propTypes = {
PropTypes.object
]),
trigger: PropTypes.string,
isInteractive: PropTypes.bool,
};

const DEFAULT_DELAYS = {
Expand All @@ -52,7 +51,6 @@ const defaultProps = {
delay: DEFAULT_DELAYS,
toggle: function () {},
trigger: 'click',
isInteractive: false,
};

function isInDOMSubtree(element, subtreeRoot) {
Expand Down Expand Up @@ -182,14 +180,16 @@ class TooltipPopoverWrapper extends React.Component {
handleDocumentClick(e) {
const triggers = this.props.trigger.split(' ');

if (this.props.isOpen && triggers.indexOf('legacy-auto-hide') > -1) {
// hide when clicking anywhere except target subtree (and itself if isInteractive)
if (!this._hideTimeout && !isInDOMSubtree(e.target, this._target) &&
!(this.props.isInteractive && isInDOMSubtree(e.target, this._popover))) {
if (triggers.indexOf('legacy') > -1 && (this.props.isOpen || isInDOMSubtree(e.target, this._target))) {
if (this._hideTimeout) {
this.clearHideTimeout();
}
if (this.props.isOpen && !isInDOMSubtree(e.target, this._popover)) {
this.hideWithDelay(e);
} else {
this.showWithDelay(e);

This comment has been minimized.

Sign in to view

Copy link
@kinke

kinke Jan 16, 2019

Contributor

This new logic doesn't replicate the legacy behaviour - a click on the target subtree now shows/hides it, something I explicitly don't want...

This comment has been minimized.

Sign in to view

Copy link
@TheSharpieOne

TheSharpieOne Jan 16, 2019 •
edited

Author Member

I may be confuses, but I thought clicking on the triggering element while the popover was open closed the popover. If this was not the case, then the only way a legacy popover is closed is by clicking off (off the target and popover). Was that the case?
Or is this a by-product of the legacy way requiring the user to add the onClick to the target and having them manage the target clicking? I believe in some cases, when clicking the button, the user's onClick was toggling as well as the popover callback which caused it to flip-flop back to the open state.

This comment has been minimized.

Sign in to view

Copy link
@kinke

kinke Jan 16, 2019

Contributor

If this was not the case, then the only way a legacy popover is closed is by clicking off (off the target and popover). Was that the case?

Yes (+ the option of setting isOpen to false from outside). And just as importantly, no click of any kind showed the popup, it was just an auto-hide, hence my original legacy-auto-hide trigger name. See https://github.com/reactstrap/reactstrap/blob/6.5.0/src/Popover.js#L118-L130 for the old logic.

This comment has been minimized.

Sign in to view

Copy link
@TheSharpieOne

TheSharpieOne Jan 16, 2019 •
edited

Author Member

Yes, but the reason e.target !== this._target && !this._target.contains(e.target) was there was because the onClick on the target was handled outside of the popover, by the user code. If it triggered the callback and the user also triggered the onClick, it would fire twice and result in the popover not closing. I guess you are saying that your previous use-case didn't have an onClick on the trigger, you were just manually opening them and using the toggle callback to close it.

And just as importantly, no click of any kind showed the popup, it was just an auto-hide,

I see how that can be, but I don't believe that many people were using it that way.

I think we should keep legacy as it is now by also bring back legacy-auto-hide as I see now that it was a more specific behavior, sorry for the confusion.

This comment has been minimized.

Sign in to view

Copy link
@kinke

kinke Jan 18, 2019

Contributor

Yeah, I'm definitely in favor of bringing back legacy-auto-hide.

but I don't believe that many people were using it that way

I don't know, but suspect I wasn't the only one (not hiding it manually from outside, but relying on the toggle callback). Those controlling it completely from outside can now use trigger="manual" and be done with it.
}
} else if (triggers.indexOf('click') > -1 && isInDOMSubtree(e.target, this._target)) {
// toggle when clicking target subtree
if (this._hideTimeout) {
this.clearHideTimeout();
}
Expand All @@ -206,7 +206,7 @@ class TooltipPopoverWrapper extends React.Component {
if (this.props.trigger) {
let triggers = this.props.trigger.split(' ');
if (triggers.indexOf('manual') === -1) {
if (triggers.indexOf('click') > -1 || triggers.indexOf('legacy-auto-hide') > -1) {
if (triggers.indexOf('click') > -1 || triggers.indexOf('legacy') > -1) {
['click', 'touchstart'].forEach(event =>
document.addEventListener(event, this.handleDocumentClick, true)
);
Expand Down
5 changes: 5 additions & 0 deletions src/utils.js
View file
Expand Up @@ -136,6 +136,11 @@ export const tagPropType = PropTypes.oneOfType([
PropTypes.func,
PropTypes.string,
PropTypes.shape({ $$typeof: PropTypes.symbol, render: PropTypes.func }),
PropTypes.arrayOf(PropTypes.oneOfType([
PropTypes.func,
PropTypes.string,
PropTypes.shape({ $$typeof: PropTypes.symbol, render: PropTypes.func }),
]))
]);

/* eslint key-spacing: ["error", { afterColon: true, align: "value" }] */
Expand Down

0 comments on commit 6b3c3ce

Please sign in to comment.