Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

fix(range-input): refine correctly when start is given #777

Merged
merged 3 commits into from
Jun 4, 2020

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented May 14, 2020

Summary

This PR fixes a wrong refinement when start is given.
fixes #776

Details

When start is given to ais-range-input and user clicks submit button without changing any input field, it broke.
It's because the submit button was calling

refine({ min: minInput, max: maxInput })

Both minInput and maxInput are assigned at @change event of input field. But when start is given, both of the input fields are already filled.

So I've updated the refine call to take it into account.

@eunjae-lee eunjae-lee requested a review from a team May 14, 2020 15:17
@ghost ghost requested review from Haroenv and yannickcr and removed request for a team May 14, 2020 15:17
@@ -11,7 +11,7 @@
>
<form
:class="suit('form')"
@submit.prevent="refine({ min: minInput, max: maxInput })"
@submit.prevent="refine({ min: minInput || values.min, max: maxInput || values.max })"

Choose a reason for hiding this comment

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

I believe that this doesn't fully fix the bug. I think that it'll fix everything when a refresh is done, but the input will still behave strangely when you hit the browser's back button.

Let's say that you a range input:

  1. You put in min: 10, max: 100
  2. You hit Go
  3. You change min to 20
  4. You hit Go again
  5. You hit the back button on the browser (with the history router the page will not reload but the state of the search changes)
  6. You hit Go again

In this scenario, the min value will be reset to 60.

I believe that this will still happen with your fix. In that scenario minValue would have been set to 60 and values.min would have been set to 50.

Choose a reason for hiding this comment

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

What if you added a watcher on value.min & value.max? In that watcher you could update minValue & maxValue every time value.min & value.max get updated by any source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this scenario, the min value will be reset to 60.
I believe that this will still happen with your fix. In that scenario minValue would have been set to 60 and values.min would have been set to 50.

Do you mean it will be reset to 20, while it should've stayed as 10, right?
You're right. I didn't think of that path. Thanks!

@Haroenv
Copy link
Contributor

Haroenv commented May 15, 2020

minInput is already a computed variable, not sure if that needs to be computed in the render again. Could you maybe start with writing a test of the behaviour which is failing now?

@eunjae-lee
Copy link
Contributor Author

minInput is already a computed variable, not sure if that needs to be computed in the render again. Could you maybe start with writing a test of the behaviour which is failing now?

I don't think minInput is a computed variable.

@change="minInput = $event.currentTarget.value"
  data() {
    return {
      minInput: undefined,
      maxInput: undefined,
    };
  },

It just stores the value from @change event handler.

And the test case I added fails without my change.

https://github.com/algolia/vue-instantsearch/pull/777/files#diff-17a43ab3d900702112db6482b0d0fc51R311-R333

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented May 15, 2020

This is the reproduction.
Note in the gif that minInput and maxInput are not changing after page transition.

They're meant to be temporary variables to store user input only until user hits "Go" button. If page, state, or whatever changes, it's safe to reset minInput and maxInput.

Kapture 2020-05-15 at 11 29 13

@Haroenv
Copy link
Contributor

Haroenv commented Jun 4, 2020

In a similar range input I made yesterday I used a watcher instead of updated to update the min & max values: https://codesandbox.io/s/thirsty-taussig-y61nc?file=/src/UnitRangeInput.vue

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Jun 4, 2020

Netlify fails due to time out, and e2e fails with connection errors. (I retried a couple of times). So I'm just merging this. (yarn run test:e2e was successful on my mac)

@eunjae-lee eunjae-lee merged commit 6eb46c8 into master Jun 4, 2020
@eunjae-lee eunjae-lee deleted the fix/range-input branch June 4, 2020 15:44
Haroenv pushed a commit to algolia/instantsearch that referenced this pull request Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Range input resets if updated after an URL change with routing
3 participants