-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dashboard) - add translations frontend and db #211
Conversation
...icsa-apps/dashboard/src/app/modules/translations/pages/edit/translations-edit.component.html
Show resolved
Hide resolved
...icsa-apps/dashboard/src/app/modules/translations/pages/edit/translations-edit.component.html
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/translations/translations.service.ts
Show resolved
Hide resolved
editActionFeedbackMessage: string; | ||
constructor(private service: TranslationDashboardService, private route: ActivatedRoute, private router: Router) { | ||
this.service.ready(); | ||
this.route.params.subscribe((params) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit(non-blocking) - Any time you create a subscription you are using browser memory. So when the subscription is no longer required you should remove it. This is commonly done by linking the subscription to ngOnInit and ngOnDestroy methods, e.g.
https://blog.bitsrc.io/6-ways-to-unsubscribe-from-observables-in-angular-ab912819a78f.
Would be good to tidy this up next time working on the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, will surely tidy up next time
apps/picsa-apps/dashboard/src/app/modules/translations/pages/home/translations.page.html
Show resolved
Hide resolved
.../picsa-apps/dashboard/src/app/modules/translations/pages/new/new-translations.component.html
Show resolved
Hide resolved
zm_ny text null, | ||
-- Primary key generated as combination of tool, context and en text | ||
constraint translations_pkey primary key (tool, context, en) | ||
) tablespace pg_default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(+) - Thanks for exporting the defintion, can confirm very smooth for me to recreate the table locally using this.
I've made a few small tweaks, to include a couple extra metadata columns and to change the way the primary key is generated. See update in commit 813935b
...d/src/services/core/supabase/components/storage-file-picker/storage-file-picker.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice implementation overall, great to see you were able to implement pretty much all aspects of backend server and dashboard (creating tables, generating types, setting up all necessary db bindings).
You'll see I've added one commit to update the db columns - as I've made the changes directly (instead of writing a migration) you'll need to reset your local db to update the changes yarn nx run picsa-server:supabase db reset
. You'll also see I've included some initial data in the seed
folder, which can be imported locally using the supabase dashboard (there's a button to import csv when you load an empty table)
As part of next tasks I think we will probably want to decide what will be the best way to allow administrators to update translations (and how to update the main list from the app), but for now I think looks really good and should provide a solid base to build from so nice work!
@khalifan-kfan - Minor update to follow previous comments. I tested implementing my proposed changes to how ids are calculated (generating at the db level by combining tool, context and en columns), however this makes all other navigation tricky because there's no longer an So instead I've re-implemented to keep an const textId = row.en.toLowerCase().replace(/[^a-z0-9]/gi, '');
return `${row.tool}-${row.context || ''}-${textId}`; I've updated the db definition and seed data accordingly, so hopefully should be clearer once imported. |
Thanks for the feedback and updates on this PR. |
Description
Enable display and editing of translations on the dashboard
Discussion
closes #208
Preview
Link to app preview if relevant
Screenshots / Videos