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): add boundary element config option (position constraint) #1439

Merged
merged 23 commits into from Dec 9, 2017

Conversation

Projects
None yet
3 participants
@tmorehouse
Member

tmorehouse commented Dec 7, 2017

Adds new prop boundary to component versions - can be either scrollParent (Popper default), window, viewport, or an element reference

Adds modifiers window and viewport to directive versions (if not present defaults to scrollParent)

Addresses #1434

A similar issue (#1163 and twbs/bootstrap#24251) applies to dropdowns, which will be addressed in a separate PR #1440 PR twbs/bootstrap#24979 addresses the issue in Bootstrap V4 official.

[toolpop.class]: add boundariesElement config
defaults to 'scrollParent' (poppoer default)

Useful values: 'window' or 'viewport', or an element reference
@codecov-io

This comment has been minimized.

codecov-io commented Dec 7, 2017

Codecov Report

Merging #1439 into dev will increase coverage by 2.77%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #1439      +/-   ##
=========================================
+ Coverage   40.73%   43.5%   +2.77%     
=========================================
  Files         135     135              
  Lines        2816    2717      -99     
  Branches      876     839      -37     
=========================================
+ Hits         1147    1182      +35     
+ Misses       1178    1097      -81     
+ Partials      491     438      -53
Impacted Files Coverage Δ
src/utils/tooltip.class.js 16.9% <ø> (+2.17%) ⬆️
src/mixins/toolpop.js 26.66% <ø> (ø) ⬆️
src/directives/popover/popover.js 51.16% <100%> (+47.52%) ⬆️
src/directives/tooltip/tooltip.js 51.16% <50%> (+47.52%) ⬆️

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 39eb237...7ec04b8. Read the comment docs.

tmorehouse added some commits Dec 7, 2017

@tmorehouse tmorehouse changed the title from feat(tooltip+popover): add boundariesElement config option (position constraint) to feat(tooltip+popover): add boundary element config option (position constraint) Dec 7, 2017

tmorehouse added some commits Dec 7, 2017

@tmjoen

This comment has been minimized.

tmjoen commented Dec 8, 2017

Does this address the problem with popovers not being centered anymore?

There was a regression somewhere between the alphas and the current version which messes with the centering of the popover.

This is from v4-alpha.6

image

And this is from 1.3.0

image

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 8, 2017

@tmjoen I believe alpha 6 used the old popover code (pre- Popper.js, using Tether.js and the old Bootstrap V4.alpha CSS). There is an issue with how the centering calculation occurred on the Bootstrap V4.beta CSS & Popper code, and there is an issue opened at twbs/bootstrap that mentioned this. See twbs/bootstrap#23793 and twbs/bootstrap#23468

This PR doesn't address that issue, it addresses the issue when the trigger element is placed inside an container element that has overflow: scroll set, which prevents the tooltip/popover from being placed correctly (due to being constrained to be inside of the scrollable element.

tmorehouse added some commits Dec 9, 2017

tmorehouse added some commits Dec 9, 2017

@tmorehouse tmorehouse merged commit 08fd7ce into dev Dec 9, 2017

4 checks passed

License Compliance License checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 75% of diff hit (target 40.73%)
Details
codecov/project 43.5% (+2.77%) compared to 39eb237
Details

@tmorehouse tmorehouse deleted the toolpop/boundaries branch Dec 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment