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): ability to programmatically show and hide tooltip and popover #1366

Merged
merged 19 commits into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@tmorehouse
Member

tmorehouse commented Nov 19, 2017

For the component versions, adds a new syncable Boolean prop show which allows the user to control the visibility state of the tooltip or popover.

For both component version and directive versions, one can use the bv::hide::{tooltip|popover} $root event to close a particular tooltip or popover by passing the trigger element's ID as the first argument. if no ID is passed, then all tooltips or popovers are closed (existing functionality)

For both component version and directive versions, one can use the bv::show::{tooltip|popover} $root event to open a particular tooltip or popover by passing the trigger element's ID as the first argument.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 19, 2017

Codecov Report

Merging #1366 into dev will decrease coverage by 0.09%.
The diff coverage is 23.8%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #1366     +/-   ##
=========================================
- Coverage   43.18%   43.08%   -0.1%     
=========================================
  Files         130      130             
  Lines        2867     2885     +18     
  Branches      885      891      +6     
=========================================
+ Hits         1238     1243      +5     
- Misses       1244     1254     +10     
- Partials      385      388      +3
Impacted Files Coverage Δ
src/mixins/toolpop.js 33.8% <11.11%> (-4.3%) ⬇️
src/utils/tooltip.class.js 14.24% <33.33%> (+1.11%) ⬆️

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 281945d...3e4bd9b. Read the comment docs.

@tmorehouse tmorehouse requested review from SirLamer and pi0 Nov 19, 2017

@tmorehouse tmorehouse changed the title from feat(tooltip+popover): programatically show amd hide tooltip and popover to [WIP] feat(tooltip+popover): programatically show amd hide tooltip and popover Nov 19, 2017

tmorehouse added some commits Nov 19, 2017

[tooltip+popover] use $root events to show/hide specific tooltip/popover
By passing and ID (of the trigger element) as the first argument to `$root.$emit('bv::{hide|show}::{tooltip|popover}, id)`, we can programmatically hide or show a particular tooltip/popover.

If no ID is passed to the hide event, then all tooltips or popovers are closed.

@tmorehouse tmorehouse changed the title from [WIP] feat(tooltip+popover): programatically show amd hide tooltip and popover to [WIP] feat(tooltip+popover): programatically show and hide tooltip and popover Nov 19, 2017

@tmorehouse tmorehouse changed the title from [WIP] feat(tooltip+popover): programatically show and hide tooltip and popover to feat(tooltip+popover): programatically show and hide tooltip and popover Nov 19, 2017

@tmorehouse tmorehouse removed the Status: WIP label Nov 19, 2017

@tmorehouse tmorehouse changed the title from feat(tooltip+popover): programatically show and hide tooltip and popover to feat(tooltip+popover): ability to programmatically show and hide tooltip and popover Nov 20, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

@SirLamer when you get a chance could you test out this PR with your usage scenario?

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

Haha well I just finished work on implementing this and was going about testing it before submitting a PR, though events-only to not disturb your current design approach. I prefer the synced prop approach anyway (when I did it before, synced props was not a Vue feature).

I'll review this but it is very late where I am right now so I'll have to kick the tires tomorrow.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

Actually I had a look-over and merged some of my work with yours instead of sleeping, OOPS. I'll give it another pass tomorrow. In particular I want to implement the "advanced reactive" example using the new 'show' prop since I think it's cleaner.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

OH, also, I caught a couple of other documentation errors and fixed those, also I very arrogantly changed the default 'placement' to 'auto' since I think that makes sense, but also because I noticed the documented default did not match the actual default which compelled me to take a look at it in the first place. I can change it back to 'top' or 'right' if you'd prefer those, though I think that would be presumptuous.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

The default placements were following bootstraps default placements (top for tooltip, and right for popover). Both will "flip" automatically if tooltip or popover is to close to the viewport edge.

this._toolpop.hide()
}
show ? this.onOpen() : this.onClose()

This comment has been minimized.

@tmorehouse
@@ -32,7 +32,7 @@ export default {
},
placement: {
type: String,
default: 'right'
default: 'auto'

This comment has been minimized.

@tmorehouse

tmorehouse Nov 20, 2017

Member

Bootstrap V4 uses a default of 'right' for their popovers.

This comment has been minimized.

@tmorehouse

tmorehouse Nov 20, 2017

Member

Although I was never overly fond of it being 'right', unless the trigger element is on the far left.

This comment has been minimized.

@SirLamer

SirLamer Nov 20, 2017

Contributor

It's fine to match Bootstrap V4, I fussed with it after noticing the docs didn't match the actual as-defined default. I'll change it to match V4.

tmorehouse and others added some commits Nov 20, 2017

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

Okay I have completed my adjustments, have at it.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

@SirLamer looks great!

I think we'll wait to merge this just before the next release goes out (as the docs site gets updated automagically from changes in the dev branch).

@tmorehouse tmorehouse requested review from alexsasharegan and mosinve Nov 20, 2017

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

Then, as an aside, perhaps the master branch should be used for the release build so that you can merge PRs to the dev build as they are completed, otherwise you are just going to face a collision mess come release.

@SirLamer

Proposed changes were submitted as branch edits.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

By the way, one issue I did not resolve is the behavior of the show-on-load popover state, since this has the same "fails to react to built-in triggers" problem as when the popover is made to programmatically display after load. In effect the user has to click the button twice to make it close. Per the issue discussion on this I didn't take it as part of the scope of this PR to resolve this general issue of event triggers causing a display state change, so I left it as it is for now. The subsequent demo I included illustrates the problem and a potential fix which can be affected externally to Popover.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

Yeh, we need to change master to be based on the BS V4.beta dev branch, rather than the old alpha branch. I think @pi0 is working on that.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

Yup, re the initial show state... Bootstrap V4 native has the same issue.

One way maybe to address it, is if the tooltip/popover has only a single "trigger" specified, then that trigger is set as the "active" trigger via the show/hide methods in the base class. But then it still has wonky effect if multiple triggers are specified (tooltip, by default, has two triggers specified)

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

Maybe it's something better fixed in Popper.js, instead of here.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

In the original Bootstrap code they set the 'manual' trigger in the active triggers tracking object, so that a tooltip/popover couldn't be closed by any other trigger, except programmatically. But that is no longer present in their current codebase.

@pi0

This comment has been minimized.

Member

pi0 commented Nov 20, 2017

Master rebased to dev and CI now deploys from master instead of dev (22e432d). So we are safe to merge (And possibly next release :D)

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

@SirLamer at least the caveat regarding manual trigger is mentioned in the docs as a known behavior.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

So we are good fer merging?

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 20, 2017

As far as I am concerned we're good.

Re the manual trigger issue, this PR change does not affect or harm anyone not using programmatic control, so I am okay with the current caveat condition and maybe a better solution will be arrived upon later.

@tmorehouse tmorehouse merged commit 360b337 into dev Nov 20, 2017

2 checks passed

License Compliance License checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse deleted the feat/toolpop branch Nov 20, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

@SirLamer What if we introduce a new trigger called none or manual (which could be used on both directive and component)?

This could disable the default triggers (regular event handlers), which means the popover can only be opened programmatically?

if none/manual is specified with any other triggers, manual/none it would trump the other triggers and take precedence.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

Providing an empty string is the same as "none", this works.

Also, I was experimenting a bit and found that the "blur" trigger works very well with programmatic control. It makes the trigger close when either the button or the popover loses focus after being opened with "show.sync".

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

I've created PR #1387 which adds a disabled prop to the component versions of tooltip and popover Take a look when you get a chance.

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