-
Notifications
You must be signed in to change notification settings - Fork 307
Feature/custom translations frontend #857
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
base: main
Are you sure you want to change the base?
Feature/custom translations frontend #857
Conversation
10cd759
to
d285bd1
Compare
4ff16fa
to
81fd1ed
Compare
src/frontend/apps/impress/src/features/language/hooks/useTranslationsCustomizer.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/language/hooks/useTranslationsCustomizer.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/language/hooks/useTranslationsCustomizer.ts
Outdated
Show resolved
Hide resolved
b9b16aa
to
06b91a1
Compare
06b91a1
to
af92119
Compare
src/frontend/apps/impress/src/features/language/hooks/useTranslationsCustomizer.ts
Outdated
Show resolved
Hide resolved
a65fca6
to
cddc429
Compare
cddc429
to
9a9b717
Compare
5f855f1
to
ed137ef
Compare
ed137ef
to
2e7f552
Compare
2e7f552
to
6a036b3
Compare
6a036b3
to
6a75453
Compare
0080fdc
to
bf65c8b
Compare
d86f811
to
f7abb03
Compare
@@ -11,5 +11,5 @@ export interface User { | |||
email: string; | |||
full_name: string; | |||
short_name: string; | |||
language: string; | |||
language?: string; |
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.
language
can be undefined from the backend ?
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.
Yes, language is nullable.
It was introduced with the language synchronization feature and should've been changed back then.
It allows us to update the users preferred language automatically from the i18n browser detected language - without overriding his existing preference.
const currentCustomTranslations: Resource = useMemo( | ||
() => currentConfig?.theme_customization?.translations || {}, | ||
[currentConfig], | ||
); | ||
|
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.
const currentCustomTranslations: Resource = useMemo( | |
() => currentConfig?.theme_customization?.translations || {}, | |
[currentConfig], | |
); | |
const currentCustomTranslations: Resource = currentConfig?.theme_customization?.translations || {}; | |
return { | ||
currentCustomTranslations, | ||
customizeTranslations, | ||
}; | ||
}; |
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.
You don't seem to need to export them.
); | ||
|
||
useEffect(() => { | ||
if (currentCustomTranslations) { |
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.
currentCustomTranslations
seems everytime defined.
if (currentCustomTranslations) { |
|
||
const availableFrontendLanguages = useMemo( | ||
() => Object.keys(i18n?.options?.resources || { en: '<- fallback' }), | ||
[i18n], |
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.
[i18n], | |
[i18n?.options?.resources], |
log('Updating backend language (%O)', { | ||
requested: language, | ||
from: user.language, | ||
to: closestBackendLanguage, | ||
}); |
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.
Should be just during the development part I think.
log('Updating frontend language (%O)', { | ||
requested: language, | ||
from: i18n.resolvedLanguage, | ||
to: closestFrontendLanguage, | ||
}); | ||
void i18n.changeLanguage(closestFrontendLanguage); |
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.
Same it is part of the development part, should be removed.
function createUser(): Promise<User> { | ||
throw new Error('Not yet implemented.'); | ||
} |
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.
We are creating things that we don't need yet (maybe never ?), same for the deleteUser.
After this feature is made to manage the authentication, I don't know if it is the good place to add the user mutation.
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 your valuable feedback on the useAuthMutation
pull request. I appreciate you highlighting the concerns around the scope and placement of the user-related mutations.
I agree with your observations, and I believe a couple of changes will clarify the intent and improve the structure:
-
Renaming for Clarity:
- You're right, the current names are a bit misleading. I propose renaming:
useAuthQuery
touseUserQuery
getMe
toreadUser
(which will default to fetching/users/me/
but could accept an ID).useAuthMutation
touseUserMutation
.
- This better reflects that we are fundamentally querying and mutating
User
data.
- You're right, the current names are a bit misleading. I propose renaming:
-
Relocation to Entity-Specific Path:
- Following the renaming, I'll move these hooks to a more appropriate location, such as
api/entities/user/
. This aligns with the idea that these are general-purpose tools for managing theUser
entity, not strictly tied to the authentication feature's codebase.
- Following the renaming, I'll move these hooks to a more appropriate location, such as
Regarding createUser
and deleteUser
:
My intention in including createUser
and deleteUser
(even as unimplemented stubs for now) within useUserMutation
was to:
- Establish a Complete CRUD Pattern: Provide a full, standard interface (Create, Read, Update, Delete) for the
User
entity from the outset. This is a common and robust pattern, similar to what one might find in ORMs, and it makes the API forUser
management predictable. - Serve as a Clear Blueprint: This structure acts as a boilerplate, clearly defining how all entity mutations should be handled. It sets an example for consistency when we add mutations for other entities in the future.
- Enhance Readability & Single Responsibility: Grouping all mutation logic for a
User
(create
,update
,delete
) withinuseUserMutation
maintains a clear single responsibility for modifying user data. I believe this makes the code more self-documenting and easier to navigate. - Future Preparedness with Minimal Overhead: While these functions aren't used by the current feature iteration, defining them now prepares us for future requirements. The stubs themselves add negligible overhead but offer significant structural benefits and clarity for ongoing development.
By renaming and relocating these hooks to focus on the User
entity, I believe the inclusion of the full CRUD set (even with stubs) becomes more logical as it defines the comprehensive API for User
management.
I've spent considerable time researching best practices for data fetching and mutations in React, and this pattern of co-locating entity-specific hooks with a complete operational set seemed the most explicit and maintainable.
Please let me know if these proposed changes and this explanation address your concerns.
I'm keen to ensure we're building a clean and scalable foundation.
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.
We have agreed to simplify it to useUpdateUser that does not follow CRUD explicitly.
currentBackendLanguage, | ||
currentFrontendLanguage, | ||
changeLanguageSynchronized, | ||
currentUser, |
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.
Is currentUser
necessary ?
[changeBackendLanguage, changeFrontendLanguage, currentUser], | ||
); | ||
|
||
useEffect(() => { |
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.
I have the feeling this useEffect is trigger often.
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.
It was not good to have the useEffect in the useSynchronizedLanguage hook anyways,
as it added 1 useEffect call, everytime useSynchronizedLanguage() is instantiated.
To counteract, one would need to develop a Provider to share Context.
Since i didn't see the need for a dedicated Provider just to prevent a race condition / enable singleton pattern, i instead moved the useEffect to the ConfigProvider and simplified useSynchronizedLanguage to only be responsible for change language.
The default behavior ("sync frontend detected language to backend if backend language is null") is now handeled in the ConfigProvider. It looks more on par with what we already have in there.
f7abb03
to
4d59da8
Compare
Part of customization PoC Signed-off-by: Robin Weber <[email protected]>
8c383a0
to
372b8d9
Compare
Part of customization PoC Signed-off-by: Robin Weber <[email protected]>
Introduces dedicated mutations (for authentication/user operations) separating them from queries to align with best practices for data fetching and state management. Queries remain responsible for READ operations, while mutations now handle CREATE, UPDATE, and DELETE actions (for user data) improving separation of concerns. Signed-off-by: Robin Weber <[email protected]>
- Refactors "useTranslationsCustomizer" to "useCustomTranslations" - Refactors "useLanguageSynchronizer" to "useSynchronizedLanguage" - Refactors "LanguagePicker" to better reflect its component role - Refactors "LanguagePicker" to use "useSynchronizedLangue" - Removes unused "useChangeUserLanguage" - To change the user language, use "useAuthMutation" instead Signed-off-by: Robin Weber <[email protected]>
- Language will only be changed if different from current language - Added test for custom translations Signed-off-by: Robin Weber <[email protected]>
372b8d9
to
bb1c96e
Compare
Adds the possibilty to inject/override custom translations at runtime.