egoist / vue-slim-tabs

A slim tab component for Vue.js (1.3 kB minified)
https://vue-slim-tabs.egoist.sh/
MIT License
103 stars 18 forks source link

Emit event on tab change #40

Open jrtitus opened 4 years ago

jrtitus commented 4 years ago

Rather than (or in addition to) using an onSelect prop: this.$emit('select', index).

Additionally, onSelect fires every time a tab is selected, even if it's already the active tab. This may not be intended behavior, so:

    switchTab(e, index, isDisabled) {
      // Only fire event/function if not currently selected
      if (!isDisabled && this.selectedIndex !== index) {
        this.selectedIndex = index;
        // Left in place for backwards compatibility
        this.onSelect && this.onSelect(e, index);
        // Not sure if the event variable is actually needed here
        this.$emit('select', index);
      }
    }
issue-label-bot[bot] commented 4 years ago

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.90. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

polosson commented 3 years ago

I think this is more likely a bug than a feature-request (at least, only the part about the onSelect prop).

Indeed, it seems that props prefixed with on are not usable with JSX.

If you do in your render() :

<Tabs onSelect={(e, index) => { console.log(e, index); }}>
  <Tab title="one">First tab</Tab>
  <Tab title="two">Last tab</Tab>
</Tabs>

Nothing happens when you switch from one tab to another! The onSelect prop inside the Tab component is undefined, even if you give it a function. Maybe this is because the on prefix is used by Vue for directives and custom events?

Anyway, it would be better to stick to what Vue (v2) recommends, that is to use an event for this, as suggested by @jrtitus:

this.$emit('select', e, index);

The advantage is that it must be backwards compatible, so in theory there is no need to touch the existing code.

An other option is to rename the prop onSelect to something like callback or whenSelected, but it would be less straightforward.

Thanks for this very cool component anyway! I'll try to submit a PR soon.

jrtitus commented 3 years ago

@polosson That JSX doesn't look exactly correct for use with Vue. I wouldn't expect console to be available in the template, and onSelect needs to be written as a bound prop (v-bind: or :) since it is not a string.

Try this?

<template>
  <Tabs :onSelect="tabSelected">
    <Tab title="one">First tab</Tab>
    <Tab title="two">Last tab</Tab>
  </Tabs>
</template>
<script>
export default {
  methods: {
    tabSelected(e, index) { console.log(e, index) }
  }
}
</script>
polosson commented 3 years ago

Thanks @jrtitus, but I really think my JSX is very valid! It's just another way of writing components, without the need of a template, by using the render() method of the component itself:

export default {
  render() {
    return (
      <Tabs onSelect={(e, index) => { console.log(e, index); }}>
        <Tab title="one">First tab</Tab>
        <Tab title="two">Last tab</Tab>
      </Tabs>
    );
  }
};

By the way, I wouldn't have made this comment if I hadn't tested the code first... :sweat_smile:

jrtitus commented 3 years ago

@polosson Thanks for the clarification. I guess I'm just not familiar with using the render() method. Sorry for doubting you!