Skip to content
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

Merged
merged 9 commits into from
Jan 15, 2024

Conversation

khalifan-kfan
Copy link
Collaborator

@khalifan-kfan khalifan-kfan commented Jan 3, 2024

Description

Enable display and editing of translations on the dashboard

Discussion

closes #208

Preview

Link to app preview if relevant

Screenshots / Videos

Screenshot 2024-01-03 at 09 24 01

@github-actions github-actions bot added the App: Dashboard Updates related to Dashboard app label Jan 3, 2024
editActionFeedbackMessage: string;
constructor(private service: TranslationDashboardService, private route: ActivatedRoute, private router: Router) {
this.service.ready();
this.route.params.subscribe((params) => {
Copy link
Collaborator

@chrismclarke chrismclarke Jan 15, 2024

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

Copy link
Collaborator Author

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

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;
Copy link
Collaborator

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

apps/picsa-server/supabase/types/index.ts Show resolved Hide resolved
Copy link
Collaborator

@chrismclarke chrismclarke left a 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!

@chrismclarke
Copy link
Collaborator

chrismclarke commented Jan 15, 2024

@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 id query parameter to search for.

So instead I've re-implemented to keep an id field in the database, although changed it instead of being a auto-generated number to a manually generated string - specifically by converting the en text to lower case without spaces and special characters, and combining with the tool and context columns

 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.

@chrismclarke chrismclarke merged commit d56cace into main Jan 15, 2024
3 checks passed
@chrismclarke chrismclarke deleted the ft-server-translations branch January 15, 2024 22:16
@chrismclarke chrismclarke changed the title Ft server translations feat(dashboard) - add translations frontend and db Jan 16, 2024
@khalifan-kfan
Copy link
Collaborator Author

Thanks for the feedback and updates on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: Dashboard Updates related to Dashboard app ready for review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(dashboard): translation management
2 participants