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(tooltip, popover): overall code refactor for better reactivity and performance (fixes: #1990, #2937, #3480, #3717, #3854, closes #3451) #3908

Merged
merged 393 commits into from
Aug 28, 2019

Conversation

tmorehouse
Copy link
Member

@tmorehouse tmorehouse commented Aug 17, 2019

Describe the PR

Complete refactor of tooltip and popover code to be more "Vue" like (less direct DOM manipulations, and no longer a direct port of Bootstrap v4.3 code), more modular, and more reactive.

Clears up many issues with tooltips and popovers

Notable improvements:

  • Component versions now render only a comment placeholder node in the DOM (instead of a DIV), so that app layout is never affected (i.e. component version can be placed directly inside input groups, etc)
  • variant and custom-class are reactive and can be changed while tooltip is open
  • custom-class can now be a scoped style, but note that one still needs the vue-loader /deep/ selector to target the inner elements of the tooltip or popover.
  • Directive versions are now reactive to title attribute changes and config option changes while the tooltip or popover is open
  • Triggers setting is now reactive. they can be changed while the tooltip is open
  • Show/hide delays for Tooltip and Popover are now defaulted to 50ms (delays are applicable for hover and focus only), which will help reduce "spastic" tooltips/popovers.
  • It is now possible to specify hide and show delays individually on directive versions via modifiers
  • It is now possible to set triggers to 'manual' which allows only programmatic showing/hiding of tooltips and popovers
  • When trigger is 'hover' (or 'focus'), one can now move the mouse over (or move focus to) the tooltip/popover and it will remain open
  • Popovers with slot content do not "breakup" when quickly hovering/un-hovering
  • User can now optionally supply their own ID to be used on the tooltip/popover root element. If an ID is not provided, an ID will automatically be added (default behaviour).
  • If a tooltip or popover is given an ID, $root events can use this ID to open/close/enable/disable the tooltip/popover. Use of the trigger element's ID works as well.

Note:

  • Tooltips an popovers now use a bit of custom SCSS to make the Bootstrap v4.3 transitions work better with Vue's <transition> component. Previously tooltips and popovers could not take advantage of Vue's <transition> component. The use of the <transition> component has greatly reduced the lines of code required, at the expense of a few new lines of custom SCSS.

Fixes #1990
Fixes #2937
Fixes #3480
Fixes #3717
Fixes #3854

Closes #3451
Closes #2261

TODO:

  • Change component b-tooltip to use new instances
  • Change component b-popover to use new instances
  • Change directive v-b-tooltip to use new instances
  • Change directive v-b-popover to use new instances
  • Tooltip/Popover without any triggers for programmatic showing/hiding only (Closes Create a tooltip without UI triggers #3451)
  • Update test suites (add reactive title/content/variant/custom-class tests)
  • Docs update

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Enhancement
  • ARIA accessibility
  • Documentation update
  • Other (please describe) Code refactor

Does this PR introduce a breaking change? (check one)

  • No
  • Yes (please describe)

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (i.e. [...] (fixes #xxx[,#xxx]), where "xxx" is the issue number)
  • It should address only one issue or feature. If adding multiple features or fixing a bug and adding a new feature, break them into separate PRs if at all possible.
  • The title should follow the Conventional Commits naming convention (i.e. fix(alert): not alerting during SSR render, docs(badge): update pill examples, fix typos, chore: fix typo in README, etc). This is very important, as the CHANGELOG is generated from these messages.

If new features/enhancement/fixes are added or changed:

  • Includes documentation updates (including updating the component's package.json for slot and event changes)
  • Includes any needed TypeScript declaration file updates
  • New/updated tests are included and passing (if required)
  • Existing test suites are passing
  • The changes have not impacted the functionality of other components or directives
  • ARIA Accessibility has been taken into consideration (Does it affect screen reader users or keyboard only users? Clickable items should be in the tab index, etc.)

If adding a new feature, or changing the functionality of an existing feature, the PR's
description above includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #3908 into dev will increase coverage by 0.5%.
The diff coverage is 99.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #3908     +/-   ##
=========================================
+ Coverage   99.33%   99.84%   +0.5%     
=========================================
  Files         234      236      +2     
  Lines        4526     4462     -64     
  Branches     1275     1250     -25     
=========================================
- Hits         4496     4455     -41     
+ Misses         24        6     -18     
+ Partials        6        1      -5
Impacted Files Coverage Δ
src/components/tooltip/tooltip.js 100% <100%> (ø) ⬆️
src/components/tooltip/helpers/bv-popper.js 100% <100%> (ø)
src/directives/popover/popover.js 100% <100%> (ø) ⬆️
.../components/popover/helpers/bv-popover-template.js 100% <100%> (ø)
src/directives/tooltip/tooltip.js 100% <100%> (ø) ⬆️
src/components/popover/popover.js 100% <100%> (ø) ⬆️
src/components/popover/helpers/bv-popover.js 100% <100%> (ø)
src/utils/bv-transition.js 100% <100%> (ø) ⬆️
.../components/tooltip/helpers/bv-tooltip-template.js 92.3% <92.3%> (ø)
src/components/tooltip/helpers/bv-tooltip.js 99.59% <99.59%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c4c89a...0cb1177. Read the comment docs.

@tmorehouse tmorehouse changed the title feat(tooltip, popover): refactor code for better reactivity feat(tooltip, popover): refactor code for better reactivity and stability Aug 23, 2019
@tmorehouse tmorehouse changed the title feat(tooltip, popover): refactor code for better reactivity and stability feat(tooltip, popover): refactor code for better reactivity and performance Aug 23, 2019
@tmorehouse
Copy link
Member Author

tmorehouse commented Aug 28, 2019

Added in ability for scoped styles to target root element of tooltips/popovers.

Targetting inner elements of the tooltip/popover will require the use of the vue-loader /deep/ (or ::v-deep or >>>) selector, just like any component

@tmorehouse
Copy link
Member Author

Now uses BVTransition functional helper component for handling the tooltip/popover transitions, which reduces code base a little bit.

@tmorehouse
Copy link
Member Author

Tweaked some of the docs formatting

@jacobmllr95 jacobmllr95 self-requested a review August 28, 2019 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment