Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add "value" to default scope slot #459

Merged
merged 1 commit into from
May 26, 2020

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented May 26, 2020

Hi 馃憢

First let me thank you for this library, we are using it on multiple projects and we are really happy about it!


We are working with Algolia Vue InstantSearch and we created a custom range slider.

It looks like this and it works fine:

<template>
  <AisRangeInput v-bind="$attrs">
    <div slot-scope="{ currentRefinement, range, refine }">
      <vue-slider
        :min="range.min"
        :max="range.max"
        :value="toValue(currentRefinement, range)"
        lazy
        contained
        tooltip="none"
        @change="(event) => refine({ min: event[0], max: event[1] })"
      />
    </div>
  </AisRangeInput>
</template>

<script>
// not interesting
</script>

However, we want to display the current slider's value while dragging and when lazy enabled (which prevent useless and costly refine() calls, to Algolia), like the following screens:

S茅lection_723

S茅lection_724

I was able to make it working by:

  • putting a reference to vue-slider component (slider)
  • creating a computed sliderValue() which returns getValue() from ref slider
  • using vue-reactive-refs since ref are not reactive
<template>
  <AisRangeInput v-bind="$attrs">
    <div slot-scope="{ currentRefinement, range, refine }">
      <vue-slider
+       ref="slider"
        :min="range.min"
        :max="range.max"
        :value="toValue(currentRefinement, range)"
        lazy
        contained
        tooltip="none"
        @change="(event) => refine({ min: event[0], max: event[1] })"
      />
    </div>
  </AisRangeInput>
</template>

<script>
export default {
+   refs: ['slider'],
    // ...
+  computed: {
+    sliderValue() {
+      return this.$refs.slider && this.$refs.slider.getValue();
+    }
+  },
}
</script>

An other alternative would be to remove lazy property and use a custom debounce function for Algolia's refine, but I don't know if it will works since refine is a scoped props... And I don't want to use a ref, the less you use them, the better you feel 馃槄

I think you will agree with me that both solution are not ideal. In this PR, I propose to send getValue() to the default slot's scope:

<vue-slider lazy>
  <div slot-scope="{ value }">
    Current value: {{ value }}
  </div>
</vue-slider>

What do you think?
Thank!

@NightCatSama
Copy link
Owner

I think it is a bit unnecessary, if you need the internal value you can listen to the dragging event.

I think getting the internal value in the slot is too limited, and there is no guarantee whether the user really needs the internal value in this scenario.

Finally, thank you for your use and suggestions, and look forward to your reply.

@NightCatSama
Copy link
Owner

I'm not sure if you don't know if dragging can get internal values, or if it can't meet your requirements.

If you think you need to get this value in the slot, then I can merge this pr, but I might change the example.

@Kocal
Copy link
Contributor Author

Kocal commented May 26, 2020

Yea I was using the dragging event, but it's not synchronized when InstantSearch clear refinements with ais-clear-refinements, you still have the old values :/

@NightCatSama NightCatSama merged commit 53563b3 into NightCatSama:master May 26, 2020
@Kocal
Copy link
Contributor Author

Kocal commented May 26, 2020

Thank you! 馃帀

@Kocal Kocal deleted the feat/scoped-default-slot branch May 26, 2020 17:38
@NightCatSama
Copy link
Owner

Has been released to version 3.1.4.

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.

None yet

2 participants