-
Notifications
You must be signed in to change notification settings - Fork 1
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
Comments
Using |
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. |
The examples The main point I wish to convey is that we don't always have a |
Yes, sometimes I do this to prevent a ref-changing anom function to trigger a random internal |
Oh, I think I got this time. So, its just a type issue, right? Changing the type to also accept |
Yes, surely! |
Filter
component setValue
propFilter
component setValue
prop
Filter
component setValue
propFilter
: setValue
prop
@matheusps just to align: we have defined what should be done, right? |
Support or Feedback
Context
Shoreline's
Filter
component currently receives asetValue
prop of the typeDispatch<SetStateAction<S>>
(whereS
can bestring
orstring[]
).This is OK if you want to create a controlled
Filter
like this:But example
A
is not enough to be used in more complex applications. The Identity team has a scenario that isn't covered by exampleA
. I will present three simplified examplesB
,C
andD
that WOULD cover Identity's scenario.Example
B
doesn't actually work because thesetValue
prop is constrained to the typeDispatch<SetStateAction<S>>
.Example
C
doesn't work either, because thesetValue
prop is constrained to the typeDispatch<SetStateAction<S>>
.The
setFilter
property'sSetStateAction<S>
type is what constraints examplesB
andC
from working:The user is obligated to handle both cases
S
and((prevState: S) => S)
. So exampleD
actually works:Suggestion
My suggestion is to not obligate the user to handle both
S
and((prevState: S) => S)
types. Instead, thesetFilter
prop type could be changed to one of the two superset (more generic) types ofDispatch<SetStateAction<S>>
:Dispatch<S>
Dispatch<((prevState: S) => S)>
If this is done, the
setFilter
prop type becomes simpler and more generic - both ExamplesA
andD
would continue working, and a new implementation simpler than exampleD
(either exampleB
orC
) would be enabled.The text was updated successfully, but these errors were encountered: