Skip to content

Adds update:show event to CTabs #57

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 2 commits into from
Feb 7, 2020
Merged

Conversation

CVeniamin
Copy link

Currently there is no event that indicate the click on a CTab.
This PR adds a update:show event for CTabs indicating index of new active CTab.

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

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

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

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the develop branch (or to a previous version branch), not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description 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)

Other information:

@CVeniamin CVeniamin requested a review from woothu February 6, 2020 15:43
@woothu
Copy link

woothu commented Feb 6, 2020

What are real-life applications for tracking tab click?

@CVeniamin
Copy link
Author

CVeniamin commented Feb 6, 2020

I have a tab with a vue2-google-maps component and I can only obtain the reference to that component after the component is rendered, i.e., it became an active tab.
This issue is caused by the v-if condition which vue2-google-maps component does not play nicely. Basically, using update:show event I track when the component can be accessed by its ref.

Still new to vue...it is possible I'm doing something wrong?!

@woothu
Copy link

woothu commented Feb 6, 2020

Interesting case

Accessing component by reference is considered bad practice because it generates tight coupling without a trace, is error-prone and hard to debug. Why do you need a reference? Can you share your code?

@CVeniamin
Copy link
Author

CVeniamin commented Feb 6, 2020

The concrete example that I'm trying to achieve is to panTo a specific location.
This example is also shown in the vue-google-maps package.

<template>
<CTabs>
  <CTab active>
   <CCard>
     ...
  </CCard>
  </CTab>
  <CTab>
    <GmapMap ref="mapRef" ...></GmapMap>
   </CTab>
  </CTabs>
</template>
<script>
export default {
  mounted () {
    // At this point, the child GmapMap has been mounted, but
    // its map has not been initialized.
    // Therefore we need to write mapRef.$mapPromise.then(() => ...)

    this.$refs.mapRef.$mapPromise.then((map) => {
      map.panTo({lat: 1.38, lng: 103.80})
    })
  }
}

In this case accessing the reference is necessary in order to have access to Google Maps API.
However, when wrapping <GmapMap> inside a CTab that is not active I can't not access the reference on mounted.
Hope this helps to understand why having update:show is a necessary "kludge" to this specific scenario. If there are ways around this I would like know.

@woothu
Copy link

woothu commented Feb 7, 2020

This example doesn't show an edge case, which couldn't be done by props. It just shows how to make access by ref (because it is not standard due to the promise).

Behavior showed in the example can be achieved by the 'center' prop:

<GmapMap
  :center="{lat:10, lng:10}"
  :zoom="7"
  map-type-id="terrain"
  style="width: 500px; height: 300px"
>
 [...]

If you ever encounter similar problem you can use watch for ref change (https://jsfiddle.net/kenberkeley/9pn1uqam/). But this is the last thing you want to do, as its as hacky as CTabs component :P

Anyway, I see this functionality, as something which might have possible applications i.e. saving tabs state. But still, it doesn't really solve this component flexibility problem. What if users would like to listen for events on tabs for some reason? We could think of a solution that solves all these problems but it would demand a change of this component API. For now, it sacrifices full flexibility for convenient API (CTab are fake components allowing making one component for tab instead of two).

I will merge your request and make some changes to it so it will emit an event on tab change, not on tab click.

Copy link

@woothu woothu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Use events named "update:{propName}" if you want the event to be able to automatically synchronize the prop passed to the component.

In this case, the component doesn't have 'show' prop so the event shouldn't suggest that component have such prop.

  1. In case of this functionality, we are interested in tab change, not tab click.

@woothu woothu merged commit 7b2c553 into coreui:dev Feb 7, 2020
@CVeniamin
Copy link
Author

@woothu, thank you for the feedback!

Indeed, I could use :center instead of panTo but I also use fitTo and fitBounds and panToBounds to zoom-in into a specific area in the map but it is not relevant.

I picked update:show event after seeing it being used in other components, next time I will take into consideration the naming.
You are correct tab change is much better than tab click.

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

Successfully merging this pull request may close these issues.

2 participants