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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resetOnMinMaxChange to the Slider component #1438

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

hiagolcm
Copy link
Contributor

@hiagolcm hiagolcm commented Apr 19, 2023

What is the purpose of this pull request?

Adds the resetOnMinMaxChange that allows the slider to keep the previous value when the min or the max are updated

What problem is this solving?

Currently in the Slider, when the minimum or maximum value changes, the entire position of the slider is reset. This can be confusing when these attributes are dynamic.

Here is a video to show the behavior

In the video, the user sets the left value to 84, but after the update the left value is set back to 22. This happens because the max changed from 109 to 110. The user expects the initial value of 84 to be maintained, even with the change in the maximum value.

How should this be manually tested?

workspace

Add this code in react/playground/Playground.tsx:
import React from 'react'

import PageHeader from '../PageHeader'
import Layout from '../Layout'
import Button from '../Button'
import Slider from '../Slider'

const SliderTest = () => {
  const [max, setMax] = React.useState(10)
  const defaultValues = [2, 8]

  const handleSubmit = e => {
    e.preventDefault()

    setMax(value => value + 10)
  }

  return (
    <>
      <form className="flex items-end mb5">
        <Button
          type="Increase max value +10"
          onClick={handleSubmit}
          variation="primary"
          size="small">
          Increase max value +10
        </Button>
      </form>

      <div className="mt8">
        <code>resetOnMinMaxChange: true</code>
        <Slider
          min={0}
          max={max}
          alwaysShowCurrentValue={false}
          range
          defaultValues={defaultValues}
        />
      </div>

      <div className="mt8">
        <code>resetOnMinMaxChange: false</code>
        <Slider
          min={0}
          max={max}
          alwaysShowCurrentValue={false}
          range
          defaultValues={defaultValues}
          resetOnMinMaxChange={false}
        />
      </div>
    </>
  )
}

const Playground = () => (
  <Layout fullWidth pageHeader={<PageHeader title="Playground" />}>
    <SliderTest />
  </Layout>
)

export default Playground

Screenshots or example usage

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

@vercel
Copy link

vercel bot commented Apr 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
styleguide ❌ Failed (Inspect) Apr 20, 2023 5:05pm

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 19, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a691044:

Sandbox Source
vtex-styleguide-pr Configuration

@danzanzini
Copy link
Contributor

Hey @hiagolcm, I've linked the app in this workspace, and it still does not work. Do I need to set up anything else besides linking?

@hiagolcm
Copy link
Contributor Author

hiagolcm commented Apr 20, 2023

Hey @hiagolcm, I've linked the app in this workspace, and it still does not work. Do I need to set up anything else besides linking?

Hey @danzanzini o/
You need to set the resetOnMinMaxChange to false on the vtex.search-result

I added a workspace in the PR description

@danzanzini
Copy link
Contributor

danzanzini commented Apr 20, 2023

@hiagolcm Thanks.
What do you think about removing the property and making this the default behavior?

In the previous behavior the information on the slider did not reflect the proper search, so I see it as a bug
image

@hiagolcm
Copy link
Contributor Author

hiagolcm commented Apr 20, 2023

@hiagolcm Thanks. What do you think about removing the property and making this the default behavior?
@danzanzini

You mean removing the resetOnMinMaxChange, right? Done! Now the default behavior is to keep the left and right values

workspace

@danzanzini danzanzini merged commit 6c8c345 into master Apr 24, 2023
@danzanzini danzanzini deleted the feature/slider-dynamic-min-max-support branch April 24, 2023 19:03
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.

3 participants