Skip to content

feat(modal): support for optionally scoped slots and new Vue.prototype.$bvModal helper #3056

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

Merged
merged 111 commits into from
Apr 14, 2019

Conversation

tmorehouse
Copy link
Member

@tmorehouse tmorehouse commented Apr 9, 2019

Description of Pull Request:

Makes many of the named slots (including the default slot) optionally scoped with the following scope:

Method or Property Description
ok() Close the modal and fires both the hide and ok events, with evt.trigger = 'ok'.
cancel() Close the modal and fires both the hide and cancel events with evt.trigger = 'cancel'.
close() Close the modal and fires the hide and 'close' events with evt.trigger = 'headerclose'.
hide(trigger) close the modal and fires the hide event with evt.trigger = trigger
visible visibility state of the modal (true or false)

Slots that are optionally scoped:

  • modal-header
  • modal-title
  • modal-footer
  • default

This is in preparation for making fully parameterized scoped modals (issue #2206, where additional data can be passed to modal when triggered to be open via events) in the future.

Also adds this.$bvModal to all Vue instances (not available when importing only the b-modal component, but is available when importing the Modal plugin), which has the following methods:

Method Arguments Description
this.$bvModal.show() id Shows the modal with specified ID
this.$bvModal.hide() id Hides the modal with specified ID
this.$bvModal.msgBoxOk() message, options Creates a modal with the message and an OK button. Returns a Promise that resolves when modal closes
this.$bvModal.msgBoxConfirm() message, options Creates a modal with the message and an OK and CANCEL button. Returns a Promise that resolve to true/false/null when modal closes

Also created utils/inspect.js helpers for inspecting variable types, etc. Can be used by other components to help with minification.

Live preview: https://deploy-preview-3056--bootstrap-vue.netlify.com/docs/components/modal

To Do:

  • docs update (more specific docs on slots, in the customization section)
  • component package.json meta update
  • docs examples
  • add a $bvModal object to the Vue.prototype for easy triggering of modals (without need for user to $emit events on $root), which optionally returns a Promise that resolves when the modal hidden (or possibly hide) event fires (if Promise is supported in browser)
  • add a new optional read/write property details to the BvModalEvent object, for future returning of data from modal to the user (for both the hide and hidden events)
  • additional tests for slots
  • tests for $bvModal Vue prototype injection

PR checklist:

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

  • Bugfix
  • Feature
  • Enhancement to an existing feature
  • ARIA accessibility
  • Documentation update
  • Other, please describe:

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

  • Yes
  • No

If yes, please describe the impact:

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 #xxxx[,#xxxx], where "xxxx" is the issue number)
  • The PR 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.
  • PR titles should following the
    Conventional Commits naming convention (i.e.
    "fix(alert): not alerting during SSR render", "docs(badge): Updated pill examples, fix typos",
    "chore: fix typo in docs", 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)
  • 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 Apr 9, 2019

Codecov Report

Merging #3056 into dev will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3056      +/-   ##
==========================================
+ Coverage   99.03%   99.06%   +0.02%     
==========================================
  Files         207      209       +2     
  Lines        3746     3836      +90     
  Branches     1123     1141      +18     
==========================================
+ Hits         3710     3800      +90     
  Misses         28       28              
  Partials        8        8
Impacted Files Coverage Δ
...c/components/modal/helpers/bv-modal-event.class.js 100% <ø> (ø) ⬆️
src/components/modal/helpers/modal-manager.js 100% <ø> (ø) ⬆️
src/utils/inspect.js 100% <100%> (ø)
src/components/modal/index.js 100% <100%> (ø) ⬆️
src/components/modal/helpers/bv-modal.js 100% <100%> (ø)
src/components/modal/modal.js 99.49% <100%> (+0.01%) ⬆️
src/utils/env.js 100% <100%> (ø) ⬆️

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 82cdd1e...aa71a77. Read the comment docs.

@tmorehouse tmorehouse changed the title feat(modal): optionally scoped slots feat(modal): add support for optionally scoped slots Apr 9, 2019
@tmorehouse tmorehouse marked this pull request as ready for review April 9, 2019 16:12
@tmorehouse tmorehouse changed the title feat(modal): add support for optionally scoped slots feat(modal): support for optionally scoped slots and vm injection Apr 10, 2019
@tmorehouse tmorehouse requested review from jacobmllr95 and pi0 April 13, 2019 17:31
@tmorehouse tmorehouse changed the title feat(modal): support for optionally scoped slots and vm injection feat(modal): support for optionally scoped slots and Vue.prototype.$bvModal helper Apr 14, 2019
@tmorehouse tmorehouse changed the title feat(modal): support for optionally scoped slots and Vue.prototype.$bvModal helper feat(modal): support for optionally scoped slots and new Vue.prototype.$bvModal helper Apr 14, 2019
@jacobmllr95 jacobmllr95 merged commit b647830 into dev Apr 14, 2019
@jacobmllr95 jacobmllr95 deleted the tmorehouse/modal-scoped branch April 14, 2019 13:50
@bbugh
Copy link
Contributor

bbugh commented Apr 18, 2019

This is awesome @tmorehouse! I've been wanting a global ok+/cancel modal for a while and never had time to stop and wrap our functionality. I needed this again today and stumbled onto this PR. It looks really good in the review app!

Is there a sense when a new version will launch containing the new $bvModal? I tried using the dev branch directly, but it doesn't look like dist is built automatically when installing the package from the github url.

Thanks!

@tmorehouse
Copy link
Member Author

Hopefully by the end of the weekend we should have it out.

@dsanders11
Copy link
Contributor

@tmorehouse, would you consider adding transitioning (corresponding to is_transitioning) to the slot scope, or changing visible to be this.is_visible || this.is_transitioning? I've found that clearing the modal content on visible leads to an undesirable graphical effect where the content is cleared because visible is false, but it's still visible on screen due to transitioning.

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

Successfully merging this pull request may close these issues.

4 participants