Persist machine configuration in presets#514
Conversation
| import { PresetData, PresetSchema } from "@/lib/preset/preset"; | ||
| import { renderUnitSymbol, renderUnitSyntax, Unit } from "@/control/units"; | ||
|
|
||
| export const previewSeparator = undefined; |
There was a problem hiding this comment.
Why do you have a const value that is always undefined? Just use undefined
There was a problem hiding this comment.
Because we might have to change this later, then we don't need to do this in multiple places - or at least, the compiler tells us where.
Currently undefined has all the information we need. It is possible that this might change in the future. Any unique value (string, symbols, null, undefined) works. undefined is just the most light weight.
Also on the other side, if you read undefined in the list. Would you be able to guess what effect this has? Using a name here makes it obvious what the desired behavior should be.
| return ( | ||
| <div className="grid grid-cols-[60%_10%_30%]"> | ||
| {entries.map((entry, i) => { | ||
| if (entry === previewSeparator) { |
There was a problem hiding this comment.
just do === undefined here
There was a problem hiding this comment.
See my comment on the above code
|
|
||
| export const presetSchema = <S extends PresetSchema>(dataSchema: S) => | ||
| z.object({ | ||
| id: z.number().int().nonnegative().optional(), |
There was a problem hiding this comment.
Why can a preset id be optional (undefined)?
There was a problem hiding this comment.
Because Presets that don't come directly from the Preset Store (imported by the user, or just an dummy value to give to pass around in the ui) might not have an id. The id only makes sense if a preset is directly handled the store.
If we force an id everywhere, sometimes it will have an incorrect dummy value, which I consider more confusing that declaring the field as optional.
Its true that one could also create a type Representing only the parts of a preset the user is directly interested in (name, date, data), that is used instead. Any opinion on this?
There was a problem hiding this comment.
Alright then that makes sense.We will keep it that way for now
There was a problem hiding this comment.
Remove the TODO Comments and write what you want to do in a ticket instead
Oshgnacknak
force-pushed
the
62-persist-config-values
branch
from
cad5a56 to
a4af569
Compare
Oshgnacknak
force-pushed
the
62-persist-config-values
branch
from
a4af569 to
f393abb
Compare
Oshgnacknak
force-pushed
the
62-persist-config-values
branch
from
f393abb to
5a19410
Compare
Oshgnacknak
force-pushed
the
62-persist-config-values
branch
from
5a19410 to
c1c5185
Compare
Oshgnacknak
force-pushed
the
62-persist-config-values
branch
from
c1c5185 to
f89645a
Compare
2 participants
#62
As discussed, the following features are not implemented yet but will become their own issue: