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

Filter: setValue prop #1504

Open
1 task done
rafarubim opened this issue Feb 8, 2024 · 7 comments · May be fixed by #1888
Open
1 task done

Filter: setValue prop #1504

rafarubim opened this issue Feb 8, 2024 · 7 comments · May be fixed by #1888
Assignees
Labels
dev Indicates that the issue or pull request involves engineering considerations

Comments

@rafarubim
Copy link

rafarubim commented Feb 8, 2024

Support or Feedback

  • I've searched for any related issues/questions on Github before submitting this request.

Context

Shoreline's Filter component currently receives a setValue prop of the type Dispatch<SetStateAction<S>> (where S can be string or string[]).

This is OK if you want to create a controlled Filter like this:

// Example A
const [filter, setFilter] = useState('option1')

return (
  <Filter
    value={filter}
    setFilter={setFilter}

But example A is not enough to be used in more complex applications. The Identity team has a scenario that isn't covered by example A. I will present three simplified examples B, C and D that WOULD cover Identity's scenario.

Example B doesn't actually work because the setValue prop is constrained to the type Dispatch<SetStateAction<S>>.

// Example B
const [filterObj, setFilterObj] = useState({ value: 'option1' })

const handleSetFilter = useCallback((newOption: string) => {
  setFilterObj({
    value: newOption
  })
}, [setFilterObj])

return (
  <Filter
    value={filterObj.value}
    setFilter={handleSetFilter} // Doesn't work because handleSetFilter is not assignable to `Dispatch<SetStateAction<string>>`

Example C doesn't work either, because the setValue prop is constrained to the type Dispatch<SetStateAction<S>>.

// Example C
const [filterObj, setFilterObj] = useState({ value: 'option1' })

const handleSetFilter = useCallback((callbackSetOption: ((prevOption: string) => string)) => {
  setFilterObj(prevFilterObj => ({
    ...prevFilterObj,
    value: callbackSetOption(prevFilterObj.value)
  }))
}, [setFilterObj])

return (
  <Filter
    value={filterObj.value}
    setFilter={handleSetFilter} // Doesn't work because handleSetFilter is not assignable to `Dispatch<SetStateAction<string>>`

The setFilter property's SetStateAction<S> type is what constraints examples B and C from working:

type SetStateAction<S> = S | ((prevState: S) => S);

The user is obligated to handle both cases S and ((prevState: S) => S). So example D actually works:

// Example D
const [filterObj, setFilterObj] = useState({ value: 'option1' })

const handleSetFilter = useCallback((setOptionAction: SetStateAction<string>) => {
  setFilterObj(prevFilterObj => {
    const newOption = typeof setOptionAction === 'function' ?
      setOptionAction(prevFilterObj.value) :
      setOptionAction

    return ({
      ...prevFilterObj,
      value: newOption,
    })
}, [setFilterObj])

return (
  <Filter
    value={filterObj.value}
    setFilter={handleSetFilter} // This finally works!

Suggestion

My suggestion is to not obligate the user to handle both S and ((prevState: S) => S) types. Instead, the setFilter prop type could be changed to one of the two superset (more generic) types of Dispatch<SetStateAction<S>>:

  • Dispatch<S>
  • Dispatch<((prevState: S) => S)>

If this is done, the setFilter prop type becomes simpler and more generic - both Examples A and D would continue working, and a new implementation simpler than example D (either example B or C) would be enabled.

@matheusps
Copy link
Contributor

matheusps commented Feb 8, 2024

Using string as the selection type is a common pattern among component libraries such as Ariakit (we use it as base), and Radix. Also, native select has a string value. I have a question to understand the use case better: Why can't you use a string state to pass to your filter? (like on the first example)

@matheusps
Copy link
Contributor

matheusps commented Feb 8, 2024

Also, you know you don't have to create a use-callback for this right? This code will work, for example:

import React from 'react';
import { Filter, FilterItem, Stack, Text } from '@vtex/shoreline';

export default function App() {
  const [state, setState] = React.useState({ value: 'blackberry' });

  return (
    <Stack>
      <Text variant="body">Value: {state.value}</Text>
      <Filter
        label="Fruits"
        value={state.value}
        setValue={(value) => setState((prevState) => ({ ...prevState, value }))}
      >
        <FilterItem value="apple">Apple</FilterItem>
        <FilterItem value="banana">Banana</FilterItem>
        <FilterItem value="blackberry">Blackberry</FilterItem>
        <FilterItem value="cramberry">Cramberry</FilterItem>
      </Filter>
    </Stack>
  );
}

Playground: https://stackblitz.com/edit/stackblitz-starters-wx4zg1?file=src%2FApp.js.

@rafarubim
Copy link
Author

rafarubim commented Feb 8, 2024

Why can't you use a string state to pass to your filter? (like on the first example)

The examples B, C and D are just a simplified version that would represent my problem. I don't think it's useful to have an object state instead of a plain string - it was just a simple way I found to convey the problem. In my scenario, I actually have an array of strings (it's a multiple Filter) and each value must be mapped to something else (e.g. Number.parseInt()) - I didn't give more details because it's out of the scope of the suggestion.

The main point I wish to convey is that we don't always have a Dispatch<SetStateAction<S>>-typed callback ready for use, and there's a way to simplify the prop type and make it more generic simultaneously

@rafarubim
Copy link
Author

you know you don't have to create a use-callback for this right?

Yes, sometimes I do this to prevent a ref-changing anom function to trigger a random internal useEffect. It's not the case with the Filter component, I guess I wanted to cover any loose tips in the examples but just made them unnecessarily complex, sorry 😅 hahahaha

@matheusps
Copy link
Contributor

matheusps commented Feb 8, 2024

Oh, I think I got this time. So, its just a type issue, right? Changing the type to also accept ((prevState: string | string[]) => void) would work for you?

@rafarubim
Copy link
Author

Yes, surely!

@davicostalf davicostalf changed the title Shoreline Filter component setValue prop Filter: Filter component setValue prop Feb 9, 2024
@davicostalf davicostalf changed the title Filter: Filter component setValue prop Filter: setValue prop Feb 9, 2024
@davicostalf davicostalf changed the title Filter: setValue prop Filter: setValue prop Feb 9, 2024
@davicostalf davicostalf added the dev Indicates that the issue or pull request involves engineering considerations label Jul 23, 2024
@davicostalf
Copy link
Contributor

davicostalf commented Aug 15, 2024

Changing the type to also accept ((prevState: string | string[]) => void)

@matheusps just to align: we have defined what should be done, right?

@beatrizmilhomem beatrizmilhomem moved this to Discussing in Shoreline Aug 16, 2024
@viniciuslagedo viniciuslagedo self-assigned this Aug 23, 2024
@viniciuslagedo viniciuslagedo linked a pull request Aug 26, 2024 that will close this issue
@lucasaarcoverde lucasaarcoverde moved this from Discussing to Active in Shoreline Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Indicates that the issue or pull request involves engineering considerations
Projects
Status: Active
Development

Successfully merging a pull request may close this issue.

4 participants