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(toggle directives): allow simple elements to use directive #651

Merged
merged 3 commits into from Jul 6, 2017

Conversation

Projects
None yet
4 participants
@mosinve
Member

mosinve commented Jul 6, 2017

sketch to test:

<div>
  <p>
  <b-btn v-b-toggle.collapse1 variant="primary">Toggle Collapse</b-btn>
  </p>
  <p v-b-toggle.collapse1 >
i can open collapse too
  </p>

  <b-collapse id="collapse1">
  <b-card>
      Collapse contents Here
  </b-card>
  </b-collapse>
</div>

@mosinve mosinve added this to the v0.19.0 milestone Jul 6, 2017

@mosinve mosinve requested review from tmorehouse, pi0 and alexsasharegan Jul 6, 2017

@alexsasharegan

alexsasharegan approved these changes Jul 6, 2017 edited

I'm new to all this (directives), but it looks like a nice change to me!

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 6, 2017

Looks good.

One extra change needed. v-b-modal also uses the _targets.js helper. Will need to alter https://github.com/bootstrap-vue/bootstrap-vue/blob/master/lib/directives/modal.js#L7-L11 to use vnode as well

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 6, 2017

True.

targets.forEach(target => {
const vm = vnode.context;
vm.$root.$emit('collapse::toggle', target);

This comment has been minimized.

@tmorehouse

tmorehouse Jul 6, 2017

Member

You could also just do:

    vnode.context.$root.$emit('collapse::toggle', target);

And save on using a variable :)

This comment has been minimized.

@tmorehouse

tmorehouse Jul 6, 2017

Member

Also in the section below, you could also replace el.__vue__ with vnode.context on line 17,

and remove the test for el.__vue__ on line 14

😃

This comment has been minimized.

@mosinve

mosinve Jul 6, 2017

Member

Yep, you absolutely right :)

mosinve added some commits Jul 6, 2017

@tmorehouse tmorehouse changed the title from [Toggle directive] allow simple elements to use directive to feat(toggle directives): allow simple elements to use directive Jul 6, 2017

@pi0

pi0 approved these changes Jul 6, 2017

@mosinve mosinve merged commit 3361911 into bootstrap-vue:master Jul 6, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@mosinve mosinve deleted the mosinve:cih-toggle-directive branch Jul 6, 2017

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