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): programmatically disable/enable tooltip or popover #1387

Merged
merged 45 commits into from Nov 22, 2017

Conversation

Projects
None yet
4 participants
@tmorehouse
Member

tmorehouse commented Nov 21, 2017

Adds a new syncable prop disabled to disabled/enable tooltip/popove componentsr. Defaults to false

Also introduces new root events for enabling/disabling all/specific tooltips/popovers (components and directives)

Addresses issue #1065

tmorehouse added some commits Nov 21, 2017

feat(tooltip+popover): add option to disable tooltip or popover (comp…
…onent versions)

Adds a new prop `disabled` to disabled/enable tooltips/popovers.

Defaults to `false`

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

@codecov-io

This comment has been minimized.

codecov-io commented Nov 21, 2017

Codecov Report

Merging #1387 into dev will decrease coverage by 0.44%.
The diff coverage is 13.63%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #1387      +/-   ##
=========================================
- Coverage   42.95%   42.5%   -0.45%     
=========================================
  Files         130     130              
  Lines        2873    2915      +42     
  Branches      889     902      +13     
=========================================
+ Hits         1234    1239       +5     
- Misses       1249    1276      +27     
- Partials      390     400      +10
Impacted Files Coverage Δ
src/utils/tooltip.class.js 14.73% <20.83%> (+0.48%) ⬆️
src/mixins/toolpop.js 26.66% <5%> (-7.14%) ⬇️

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 6687efa...c2d8f79. Read the comment docs.

tmorehouse added some commits Nov 21, 2017

@tmorehouse tmorehouse changed the title from feat(tooltip+popover): add option to disable tooltip or popover (component versions) to feat(tooltip+popover): add prop to disable tooltip or popover (component versions) Nov 21, 2017

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

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

Maybe disable should freeze it in state, open or close? It should just disable triggers and programmatic control should still work.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

Hmmmm.... that might be an option... but the triggers are passed into the tooltip/popover class instance, so are not directly available in the component versions... unless we dip into the $activeTriggers property on this._toolpop and add a new value (i.e. manual: true), but that would only work to prevent it from closing via a trigger

This PR was mainly to address issue #1065

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

Well that's cool, keeping it simple is good. The affect of forcing it open can be done with "show.sync" and an empty "triggers" string

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

From the bootstrap V4 docs (http://getbootstrap.com/docs/4.0/components/tooltips/#methods):

.tooltip('disable')
Removes the ability for an element’s tooltip to be shown. The tooltip will only be able to be shown if it is re-enabled.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

ok well maybe our prop should be "disable" instead of "disabled"? The problem is HTML spec uses "disabled" so...

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

We use disabled in a lot of other components (i.e. dropdowns, form-inputs, pagination, etc)

Since the attribute (prop) is on a custom component it shouldn't conflict with native attributes.

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

I meant that it's best for the word spelling to match common use. I agree there is no collision problem.

Yeah "disabled" is fine

@SirLamer

This comment has been minimized.

Contributor

SirLamer commented Nov 21, 2017

btw I don't have a chance to run use tests with this today, I had a look and it looks good but I haven't done functional testing.

@SirLamer

Read-through review only, functional testing not performed.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

Might be able to add disable() and enable() methods in the class instance, which would then be called to disable/enable the tooltip/popover. Which then we could create $root listeners for events to trigger the disabling/enabling as well.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

Quickly looking at the bootstrap V4 code, it looks like if the tooltip/popover is visible when disabled, it remains visible.

I'll re-implement by adding disable and enable methods on the base class instance.

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

@tmorehouse tmorehouse changed the title from [WIP] feat(tooltip+popover): add prop to disable tooltip or popover (component versions) to feat(tooltip+popover): add prop to disable tooltip or popover (component versions) Nov 21, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 21, 2017

@SirLamer I think everything should be all hunky-dory now.

tmorehouse added some commits Nov 21, 2017

@tmorehouse tmorehouse changed the title from feat(tooltip+popover): add prop to disable tooltip or popover (component versions) to feat(tooltip+popover): programmatically disable/enable tooltip or popover Nov 22, 2017

mosinve and others added some commits Nov 22, 2017

methods: {
disableByRoot() {
if (this.disabled){
this.$root.$emit('bv::enable::popover')

This comment has been minimized.

@tmorehouse

tmorehouse Nov 22, 2017

Member

these emit events need the trigger button's ID string (the id of the button that has the tooltip), otherwise it enables or disables all tooltips

Also the event should be bv::{enable|disable}::tooltip for tooltips 😉

This comment has been minimized.

@mosinve

mosinve Nov 22, 2017

Member

Yep, true

if (this.disabled){
this.$root.$emit('bv::enable::popover')
}else{
this.$root.$emit('bv::disable::popover')

This comment has been minimized.

@tmorehouse

tmorehouse Nov 22, 2017

Member

ditto for here 😜

tmorehouse and others added some commits Nov 22, 2017

} else {
this.$root.$emit('bv::disable::tooltip', 'tooltipButton-disable')
this.$refs.tooltip.$emit('disable')

This comment has been minimized.

@mosinve

mosinve Nov 22, 2017

Member

should Popover docs be changed this way too?

@mosinve

This comment has been minimized.

Member

mosinve commented Nov 22, 2017

I don't see any issues by now.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 22, 2017

We can merge into dev, and adjust later if needed.

@tmorehouse tmorehouse merged commit 8104cb4 into dev Nov 22, 2017

2 checks passed

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

@tmorehouse tmorehouse deleted the toolpop/disable branch Nov 22, 2017

pi0 added a commit that referenced this pull request Nov 29, 2017

feat(tooltip+popover): programmatically disable/enable tooltip or pop…
…over (#1387)

* feat(tooltip+popover): add option to disable tooltip or popover (component versions)

Adds a new prop `disabled` to disabled/enable tooltips/popovers.

Defaults to `false`

* ESLint

* [b-popover] update docs

* [b-tooltip] document disabled prop

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* [tooltip.class.js] add enble and disable methods

* Update tooltip.class.js

* [toolpop.js mixin] use class instance methods for enable/disable

* [tooltip.class] add root listeners for enable/disable

* [b-popover] doc update

* [toolpop.js mixin] make disabled prop syncable

* [toolpop.js] prevent possible endless enent loops

Just in case a user fires `enabled` rather than `enable` or `disabled` instead of `disable`

* [tooltip.class] emit root enabled/disabled events and callbacks

* Update README.md

* Update package.json

* Update package.json

* Update package.json

* Update README.md

* Update tooltip.class.js

* Update README.md

* [tooltip directive] docs update

* [popover directive] docs update

* Update README.md

* Update package.json

* Update package.json

* Update README.md

* [b-tooltip] add missing triggers section in tooltip docs

* typo fix

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Fix(docs) spellcheck & formatting

* Fix(docs) formatting

* Fix(docs) formatting

* Update README.md

* Update README.md

* Update README.md

* fix(Popover docs) update 'disable Popover' example

* fix(Tooltip docs) update 'disable Tooltip' example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment