Conversation
cjs8487
left a comment
cjs8487
left a comment
There was a problem hiding this comment.
Only did a code pass for now.
Some small non-specific comments
- It looks like prettier wasn't run on some files, I'm noticing some missing spaces, especially between braces
- I'm trying to phase out usage of
useApiprogressively. We should try to avoid introducing new instances of it. I've commented on it where I saw it with some suggested fixes, but if I missed any they should all be removed if possible
Beyond that, I'm not sure I'm happy with the overall approach we settled on. I left a couple comments about some ideas where relevant, but overall, I'm wondering if we should just bite the bullet and make a database model for this. It might be nice for adding additional features on top of this in the future, such as user level default languages where we allow users to select what language they want to play in by default if the language is available for the game. Alternatively, we could probably get away with just strictly indexing the array in the database (which is already done for us, the order is stable), and making all the operations indexed. They wouldn't be idempotent in this case but that's not something that's stopped us before.
| gameId: game3.id, | ||
| goal: `Goal ${i + 1}`, | ||
| description: `Description for Goal ${i + 1}`, | ||
| translations: {German: `Ziel ${i + 1}`}, |
There was a problem hiding this comment.
These goals have translations, but the game doesn't have a language specified at the top level
| @@ -0,0 +1,11 @@ | |||
| // This file must be a module, so we include an empty export. | |||
There was a problem hiding this comment.
I don't know if I would put this file in the @types folder. We only have it in order to override/augment types in the actual @types namespace, which this wouldn't fall under. In the variants branch i had it in a prismajson.d.ts file at the top level, but I don't think it ultimately matters where we settle on putting the file or what it's called
| error: 'Translations must be an array', | ||
| }); | ||
| } | ||
| const oldTranslations = (await prisma.game.findUnique({ where: { slug } })) |
There was a problem hiding this comment.
Most (if not all) of this logic should be moved to one or more functions in a file in the database folder, Especially in this case where the function would be more than a simple wrapper around the prisma api. It makes the behavior more testable (when we eventually write the missing tests) by separating API responses from actual logic, and gives us a simple function we can easily call if we ever need to update translations from elsewhere (i.e. a different API endpoint) in the future).
| return; | ||
| }); | ||
|
|
||
| translationsRouter.post('/:slug/translations', async (req, res) => { |
There was a problem hiding this comment.
Should this post and the delete actually be a single PUT? I know they don't have precisely the same logic, but given that we're not indexing into the array for deletes or updates, I'm inclined to think that requiring the entire array to be resented when making changes may actually be reasonable here to help reduce the overhead of these functions, which are getting to be a little awful
| "@redocly/cli": "^1.34.2", | ||
| "@types/cors": "^2.8.17", | ||
| "better-sqlite3": "^11.7.0", | ||
| "@redocly/cli": "^1.34.5", |
There was a problem hiding this comment.
Some of these changes feel incorrect, like we definitely shouldn't be removed the prisma client
| setRoomData(roomData); | ||
| }, []); | ||
|
|
||
| const { data: gameData, isLoading: loadingGames } = useApi<Game>( |
There was a problem hiding this comment.
Avoid using useApi here unless you're going to lower the refresh threshold (or disable it entirely). This context in particular can be very heavy, and automated re-fetches by the api hook may cause the page to be unresponsive on lower end machines
|
|
||
| useEffect(() => { | ||
| if (gameData?.defaultLanguage && !language) { | ||
| setLanguage(gameData.defaultLanguage); |
There was a problem hiding this comment.
Can we ship the language data as part of the room data packet when the user connects or when we fetch the initial room data in the server component, rather than grabbing it this way? We should opt for using existing packets/fetches whenever possible, as even through the browser/web server will do it's best to optimize requests, we should still exercise some best practices
| canModerate: boolean; | ||
| selectedGoal: Goal | undefined; | ||
| goals: Goal[]; | ||
| language: string; |
There was a problem hiding this comment.
Should language be a part of the Settings struct?
| mutate: mutateGoals, | ||
| } = useApi<Goal[]>(`/api/games/${slug}/goals`); | ||
|
|
||
| const { data: game, isLoading: gameLoading } = useApi<Game>( |
There was a problem hiding this comment.
This should be fetched server side and passed in instead. It will automatically be batched/deduplicated with the identical requests from other pages in the game workspace. Once this is done, you'll likely want to move the remainder of translation operations to server actions in order to update the web server cache
| setCatList(cats); | ||
| }, [goals]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Once game info is fetched server side this will no longer need to be an effect, you can just pass the default language to useState
2 participants
This allows bingo creators to add translatable languages to a game.
The translation of goals can be accessed by moderators by selecting the language in the goal editor settings.
Adds a new language dropdown to the room which shows the corresponding translated goal in the selected language