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.

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

feat(Popper): Add fade support for ToolTip and Popover (#363) #1364

Merged
merged 1 commit into from Jan 16, 2019
Merged

feat(Popper): Add fade support for ToolTip and Popover (#363) #1364

merged 1 commit into from Jan 16, 2019

Conversation

esoh
Copy link
Contributor

@esoh esoh commented Jan 12, 2019

  • Bug fix
  • New feature
  • Chore
  • Breaking change
  • There is an open issue which this change addresses
  • I have read the CONTRIBUTING document.
  • My commits follow the Git Commit Guidelines
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Hi, this is my first PR. I hope I'm doing everything right! This is to address issue #363.

Changes in this PR

  • default behavior for PopperContent set to fade in and out, toggleable with prop fade={false} in Tooltip/Popover
  • default hide delay changed from 250 to 0 since now it fades out as consistent with bootstrap
  • separated the popperClasses (e.g. tooltip show) from any extra classNames as to apply these classes to the ReactPopper child component for display purposes, and the extra classes will be applied to the root Fade component. I was not sure if this is the intended behavior, or if the extra classNames should be applied to ReactPopper instead.
  • updated failing TooltipPopoverWrapper isOpen toggle test to account for the 150ms fade animation and added a test with fade={false}

@TheSharpieOne
Copy link
Member

Thank you for tackling this issue. It's something that has been asked for several times in the past but keeps getting overlooked.

I did notice a few things. First, all of the tooltips and popovers always appear as they have placement of bottom, no matter what the placement value is (See https://deploy-preview-1364--reactstrap.netlify.com/components/tooltips/#Tooltips-List for example)
Also, with autohide={false} on the tooltip, it should allow the tooltip to be remain visible while hovering over the tooltip (to select the text or something). While it does do is when moving very fast, once the fade starts, hovering over the tooltip doesn't stop it from fading. I feel as if the fade should stop and the tooltip should stay open (at 100 opacity) in this case, if possible.

adds propType `fade=true` to PopperContent used in Tooltip and Popover.
Default hide delay is changed from 250 to 0 to match bootstrap.
popperClassName, used for displaying the popper, is separated from classNames.

Closes #363
@esoh
Copy link
Contributor Author

esoh commented Jan 15, 2019

Thank you for pointing out the issues. I believe I've fixed the ones that you've mentioned, but please let me know if I'm missing anything.

@TheSharpieOne
Copy link
Member

travis is messed up, closing and reopening to trigger a build

@TheSharpieOne TheSharpieOne reopened this Jan 16, 2019
@TheSharpieOne TheSharpieOne merged commit ee15c86 into reactstrap:master Jan 16, 2019
@esoh esoh deleted the popper-fade branch January 17, 2019 04:59
juanmaguitar added a commit to juanmaguitar/reactstrap that referenced this pull request Jan 18, 2019
* '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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants