Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tackle issue #1349 (backward-compatible Popover behavior) #1360

Merged
merged 2 commits into from
Jan 10, 2019
Merged

Tackle issue #1349 (backward-compatible Popover behavior) #1360

merged 2 commits into from
Jan 10, 2019

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Jan 9, 2019

Via <Popover trigger="manual" isInteractive />.

Changed semantics for trigger="manual": hide popover when clicking outside target element and, depending on the new isInteractive prop, outside the popover itself.

Would probably need some tests & documentation.

@kinke kinke mentioned this pull request Jan 9, 2019
Closed
@TheSharpieOne
Copy link
Member

"manual" (based on the definition from bootstrap itself) seems like it would mean that the user of the library is controlling the state of isOpen themselves outside of the anything popover is doing, I don't think it would make sense to use that in this context and it seems like it work diverge from bootstrap.
It would be nicer to have something like trigger="classic" or trigger="reactstrap" to invoke the previous behavior.

@kinke
Copy link
Contributor Author

kinke commented Jan 9, 2019

It would be nicer to have something like trigger="classic" or trigger="reactstrap" to invoke the previous behavior.

Pre-v7, there was no trigger, so isOpen was controlled outside. But the popover toggled itself via a click anywhere except its target's subtree and its own. Not a real trigger IMHO, but I found something like trigger="manual" autoHide={true} too inconvenient; something like trigger="legacy-auto-hide" may not be that bad (and may make even sense in combination with other triggers like click => show on target click, but don't hide again on 2nd target click, instead require a click somewhere else).

Btw, the isInteractive prop is 'inspired' by react-tippy's interactive.

@TheSharpieOne
Copy link
Member

trigger="legacy-auto-hide" or even just trigger="legacy" sounds good to me.

Martin Kinkelin added 2 commits January 10, 2019 12:46
Restore legacy Popover behavior via `trigger="legacy-auto-hide" isInt…
5ae4c97 
…eractive delay={0}`

The new trigger hides the popover when clicking anywhere except the target element
and, depending on the new `isInteractive` prop, the popover itself.
It can be combined with the `click` trigger, in which case the popover is shown
when clicking the target element and hidden when clicking somewhere else.

Tackles issue reactstrap#1349.
Fix oversights in TooltipPopoverWrapper
81f0bd6
@kinke
Copy link
Contributor Author

kinke commented Jan 10, 2019

Alright, reworked so that <Popover trigger="legacy-auto-hide" isInteractive delay={0} /> should restore full legacy behavior. A combined trigger="click legacy-auto-hide" [isInteractive] should work as well and change which clicks hide the popover.

@TheSharpieOne
Copy link
Member

Awesome, thanks for tackling this. If you can, please add some tests and update the documentation to point out this trigger which is outside of bootstrap's spec. Or, I can merge it and add the tests and update the docs.

@kinke
Copy link
Contributor Author

kinke commented Jan 10, 2019

I'd prefer the latter ;), as I'd have to look up all the needed stuff. - Thanks!

@TheSharpieOne TheSharpieOne merged commit 1d5ce83 into reactstrap:master Jan 10, 2019
@kinke kinke deleted the legacyPopover branch January 10, 2019 16:52
juanmaguitar added a commit to juanmaguitar/reactstrap that referenced this pull request Jan 18, 2019
@juanmaguitar
Merge branch 'master' of github.com:reactstrap/reactstrap
41f8a38 
* 'master' of github.com:reactstrap/reactstrap: (81 commits)
  fix(Modal): set Modal.openCount floor to 0 (reactstrap#1368)
  feat(Popover): add default toggleable fade support (reactstrap#1364) (reactstrap#1364)
  chore(release): release 7.1.0
  feat(Popover): add legacy trigger, replacing isInteractive prop
  feat(Popover): backward-compatible Popover behavior (reactstrap#1360)
  Fix(Modal): don't treat clicks on Portal children as backdrop clicks (reactstrap#1359)
  feat(*): support forwardRef components as tag
  fix(NavLink): console error while using @reach/Router (reactstrap#1350)
  feat(Spinner): Add spinner component (reactstrap#1352)
  feat(Switch): Add support for CustomInput type='switch' (reactstrap#1353)
  chore(release): adding 7.0.2
  fix(npm): fix published files
  chore(release): adding 7.0.1
  fix(*): fix release artifacts (reactstrap#1345)
  chore(release): adding 7.0.0 (reactstrap#1342)
  docs(typo): Fix misspelling in documentation (Alerts) (reactstrap#1329)
  docs(Input): static input error (reactstrap#1335)
  fix lint issue
  Bug/v7 merge conflict fix (reactstrap#1324)
  feat(CardBody): add innerRef to CardBody (reactstrap#1318)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kinke @TheSharpieOne