Conversation
🦋 Changeset detectedLatest commit: 1f9c4b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
flagging src/routes/+layout.server.ts, unsure if its super duper necessary may be a better way to do that? if so lmk |
DarthGigi
left a comment
DarthGigi
left a comment
There was a problem hiding this comment.
There are some things that aren't quite right or not following the code/codebase standards. But that's fine, ur new and are here to learn :)
If you need help and/or have any questions, feel free to @ or dm on on Discord
There was a problem hiding this comment.
We don't use +page.server.ts or +layour.server.ts for data loading anymore (except when we need SSR, like for embeds).
We use Remote Functions
There was a problem hiding this comment.
This entire file should be deleted now.
A new enchants context should be defined in createContext.svelte.ts so it matches the implementation of both Packs and AllStats constants loading
There was a problem hiding this comment.
This is not a constant but a helper/utility, so it should be placed in $lib/shared/helper.ts (and the file deleted like I said)
There was a problem hiding this comment.
You should look at how we handle AllStats (setAllStatsContext) and do the same for enchantments.
There was a problem hiding this comment.
This should not be loaded in the +layout file as its only needed in 1 place, /stats
|
Also, I'd add a patch changeset for this |
There was a problem hiding this comment.
This should not be loaded in the +layout file as its only needed in 1 place, /stats
There was a problem hiding this comment.
This is not a constant but a helper/utility, so it should be placed in $lib/shared/helper.ts (and the file deleted like I said)
DarthGigi
left a comment
DarthGigi
left a comment
There was a problem hiding this comment.
I think these are the last changes needed?
| } | ||
|
|
||
| export class EnchantmentsContext { | ||
| #current: string[] = $state([]); |
There was a problem hiding this comment.
string[]? If some data comes from the api it should use the types from orval. I think it's ListEnchantmentsResponse in this case? Unsure tho you'd have to verify
| data: ModelsStatsOutput; | ||
| allStats: ModelsStatData[]; | ||
| combined: ModelsCombinedOutput | null; | ||
| enchantments: string[]; |
There was a problem hiding this comment.
| data: ModelsStatsOutput; | ||
| allStats: ModelsStatData[]; | ||
| combined: ModelsCombinedOutput | null; | ||
| enchantments: listEnchantmentsResponse["data"]; |
There was a problem hiding this comment.
I was referring to ListEnchantmentsResponse from orval-generated-zod.ts, we don't really use stuff like ["data"].
I think it should be typeof ListEnchantmentsResponse? I don't quite remember how to "convert" a zod type to a typescript type off the top of my head
2 participants
Description
Moving the const list of enchantments into the backend (view other pr here)
Type of Change
Related Issues
Changes Made
Screenshots/Videos
Testing
Test Environment
Checklist
Code Quality Checks
pnpm lintpasses without errorspnpm formathas been run (or no formatting issues exist)pnpm buildcompletes successfullyAdditional Notes
Deployment Notes