fix: custom bridge and exchange select indicator #130
Conversation
| const { enabledBridges, enabledExchanges } = useSettingsStore((state) => { | ||
| return { | ||
| enabledBridges: state.enabledBridges, | ||
| enabledExchanges: state.enabledExchanges, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Please use an array and shallow equality function, see other usages of zustand in the app.
There was a problem hiding this comment.
okay, checking it out
There was a problem hiding this comment.
I've pushed the changes, I get the importance of shallow but a personal opinion, I prefer Object > Array for de-structuring because with objects there isn't any positional values involved, meaning it's differentiated by the unique keys.
But with arrays, wrong positions could lead to incorrect values.
Is there any particular reason to prefer array in a case where we are just reading the values from zustand store?
There was a problem hiding this comment.
It is the way it works with some other stuff in react like const [state, setState] = useState() and also just less code with no need for mapping, etc. Maybe it is also more performant than the object, but I didn't check that.
There was a problem hiding this comment.
Correct, but ideally in react the pattern follow something like the first element is state and the second element is the handler to update the state.
But in this case we are only reading the states from Settings store. Particularly in this scenario, I wanted to know if having it in array helps.
|
I don't see any changes to the code 👀 |
…e/widget into lf-1639-custom-settings-indicator
Hey, forgot to push changes, haha. |
|
Also, I hope you've tested light theme 😄 |
We need to reset this as well? |
CleanShot.2023-09-18.at.23.28.27.mp4 |
I'm not sure that we need to reset the theme settings - for some users dark theme can be considered as default. 🤔 Let's leave this button only for route stuff for now. |
Alright, removing the reset functionality from theme CleanShot.2023-09-20.at.13.22.30.mp4 |
| @@ -98,10 +99,11 @@ export const useSettingsStore = createWithEqualityFn<SettingsState>( | |||
| ), | |||
| })), | |||
| reset: (config, bridges, exchanges) => { | |||
| const { appearance, ...restDefaultsettings } = defaultSettings; | |||
There was a problem hiding this comment.
Please restDefaultsettings -> restDefaultSettings
|
pushed all the review changes @chybisov |
CleanShot.2023-09-15.at.14.53.58.mp4