Conversation
|
src/components/InputSwitch/index.tsx
Outdated
| const handleInputChange = (s: string) => { | ||
| if (!selectedRecipeId) return; | ||
| setValue(s); | ||
| updateRecipeObj(selectedRecipeId, { [id]: s }); |
There was a problem hiding this comment.
since the code for handleInputChange and handleSelectChange are identical, maybe we could just have one?
There was a problem hiding this comment.
I consolidated all three change handlers!
src/state/store.ts
Outdated
| } | ||
| get().updateRecipeString(recipeId, JSON.stringify(obj, null, 2)); | ||
| } catch { | ||
| /* ignore */ |
There was a problem hiding this comment.
Do you think it's worth adding a console log or anything here? Just as we're continuing to add more recipes and potentially face more issues, perhaps it could be good to have some error logs
There was a problem hiding this comment.
Sure, don't know how involved to get on this PR but at least a log works!
src/components/RecipeForm/index.tsx
Outdated
| variant="filled" | ||
| disabled={!submitEnabled} | ||
| style={{ width: '100%' }} | ||
| disabled={isPacking || !submitEnabled} |
There was a problem hiding this comment.
I believe the new isPacking removes the need for submitEnabled, so I think we could remove submitEnabled entirely
src/utils/gradient.ts
Outdated
| return { currentGradient: defaultValue, selectedOption: { value: defaultValue, display_name: defaultValue, path: "" } as GradientOption }; | ||
| } | ||
|
|
||
| // 2) Shared selector path (all options for this control share it) |
There was a problem hiding this comment.
nit: it looks like you skipped 1)
There was a problem hiding this comment.
Oops yeah edited out and didn't fix the other numbers!
src/state/store.ts
Outdated
| updateRecipeString: (recipeId: string, newString: string) => void; | ||
| updateRecipeObj: (recipeId: string, updates: Record<string, string | number>) => void; | ||
| restoreRecipeDefault: (recipeId: string) => void; | ||
| getCurrentValue: (path: string) => string | number | undefined; // STABLE |
There was a problem hiding this comment.
could you help me understand the STABLE comment a bit more? from what I can tell, we have values in recipes that are objects and arrays (e.g. color, gradient.direction, etc), so it seems like lodashGet in getCurrentValue could return more than str or num.
(Oh i see, we have type guard to handle non-scalar values in GradientInput, but not in InputSwitch. Should we add a type guard in getCurrentValue itself, similar to what getOriginalValue has?)
There was a problem hiding this comment.
Not sure that's really a useful comment, I'm going to remove it.
I think you're probably right that we should also have a type guard there to be sure we are using the library carefully!
rugeli
left a comment
rugeli
left a comment
There was a problem hiding this comment.
Looks good, thanks for the initial work! It took me a bit to trace through the workflow (jsonToString-store-componets-edits). only had a non-blocking question about lodash return values and type guard along the way.
Thanks for the review! I'm sure we can continue to refine this over time, but it's something to build off of. :) |
3 participants
Problem
Closes #95
Advances #83
Advances #97
Solution
Locally state that is passed as props and spread across multiple components reliably leads to bugs and anti-patterns.
In this PR we establish a
zustandstore for the recipe data, and a few pieces of UI related state. Packing state, is an example of something that could be moved into the store in the future.Primarily this PR established the store and its use across the components.
It also runs a prefetch where on load all the recipe data is fetched from firebase and stored, to prevent numerous round trips to the back end. A small bit of "Loading..." ui read out was added to account for this while the page is loading.
Establishing the store as a clearinghouse for shared recipe data is a good first step to migrating away from using the JSON string as the single source of truth.
This PR installs and uses
lodash-es'ssetandgetfunctionality for working with the properties on the JSON in lieu of building custom utilities.