Sprint 1-2 #315
Sprint 1-2 #315
Conversation
Co-authored-by: Lucas Maupin <[email protected]>
Co-authored-by: Lucas Maupin <[email protected]>
Co-authored-by: Linda Malm <[email protected]> Co-authored-by: Sandra Larsson <[email protected]>
Co-authored-by: Linda Malm <[email protected]> Co-authored-by: Sandra Larsson <[email protected]>
Co-authored-by: lucasmaupin <[email protected]>
Co-authored-by: Saelmala <[email protected]>
There was a problem hiding this comment.
I'm not going to block merging to main, but I think there are a few things that should be changed before this goes core.
I might be wrong about the watch comments and how it triggers re-renders (I might remember wrong), but I think it would still make sense to rework it to use useWatch on single fields to avoid unnecessary re-renders.
src/api/api.ts
Outdated
| export type TBasicProductionResponse = { | ||
| name: string; | ||
| productionId: string; | ||
| lines?: TLine[]; | ||
| }; |
There was a problem hiding this comment.
TFetchProductionResponse exists which has lines as well, could this type be reused instead of introducing optional lines on the basic type?
| store.write(key, value); | ||
| }; | ||
|
|
||
| const clearStorage = (key: keyof Schema) => { |
There was a problem hiding this comment.
NITPICK: from the naming it sounds like this method would clear the entire storage
| useEffect(() => { | ||
| if (isSomeoneSpeaking) { | ||
| if (!shouldReduceVolume) { | ||
| startTimeoutRef.current = setTimeout(() => { | ||
| setShouldReduceVolume(true); | ||
| }, 1000); | ||
| } | ||
| } else { | ||
| if (startTimeoutRef.current) { | ||
| clearTimeout(startTimeoutRef.current); | ||
| startTimeoutRef.current = null; | ||
| } | ||
|
|
||
| if (shouldReduceVolume) { | ||
| setShouldReduceVolume(false); | ||
| } | ||
| } | ||
| }, [isSomeoneSpeaking, shouldReduceVolume]); |
There was a problem hiding this comment.
this effect needs to clear the timeout when the component is de-rendered
| }); | ||
|
|
||
| // Watch all form values | ||
| const watchedValues = watch(); |
There was a problem hiding this comment.
would it make sense to useWatch here instead?
There was a problem hiding this comment.
I think this file should be converted to use react-hook-form instead of the custom input/error handling
src/hooks/use-local-user-settings.ts
Outdated
| payload, | ||
| }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
please rework hook to ensure this is not needed, it indicates issues with state management and can cause bugs now or down the line
Co-authored-by: Martin Stark <[email protected]>
| // Create a store of the desired type. If it is not available, | ||
| // in-memory storage will be used as a fallback. | ||
| const store = createStorage<Schema>({ | ||
| type: StorageType.LOCAL, | ||
| prefix: "id", | ||
| silent: true, | ||
| }); | ||
|
|
||
| export function useStorage() { | ||
| const [store, setStore] = useState<StorageTS<Schema>>(); | ||
|
|
||
| useEffect(() => { | ||
| // Create a store of the desired type. If it is not available, | ||
| // in-memory storage will be used as a fallback. | ||
| setStore( | ||
| createStorage<Schema>({ |
There was a problem hiding this comment.
I think it's reasonable that the store is a global, we don't need to create a new store each time useStorage is mounted in different components. If we have several instances of store, they all manipulate the same global localstorage anyway, unless their id is unique.
There was a problem hiding this comment.
Currently we only need one store and this is to prevent a bunch of updates when subscribing to the returned functions. +1
| const onSubmit: SubmitHandler<FormValues> = (value) => { | ||
| setLoading(true); | ||
| API.createProduction({ | ||
| name: value.productionName, | ||
| lines: [ | ||
| { | ||
| name: value.defaultLine, | ||
| programOutputLine: value.defaultLineProgramOutput, | ||
| }, | ||
| ...value.lines, | ||
| ], | ||
| }) | ||
| .then((v) => { | ||
| setCreatedProductionId(v.productionId); | ||
| setLoading(false); | ||
| }) | ||
| .catch((error) => { | ||
| dispatch({ | ||
| type: "ERROR", | ||
| payload: error, | ||
| }); | ||
| setLoading(false); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
I realize this isn't new code, but all async side effects should be made inside a useEffect hook so that they can be cleaned up if the component is unmounted while the async action is ongoing.
The way to achieve that here would be to check formState: { isSubmitted } or submitCount state, which in turn triggers a useEffect hook that makes the API call. The effect could then have if (aborted) guards so that we don't try to call setCreatedProductionId or dispatch an error if the component has been unmounted.
No description provided.