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

fix: your name translation #14863

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Conversation

smitgol
Copy link
Contributor

@smitgol smitgol commented May 2, 2024

What does this PR do?

Fixes #14734

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Copy link

vercel bot commented May 2, 2024

@smitgol is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented May 2, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the i18n area: i18n, translations label May 2, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@dosubot dosubot bot added ui area: UI, frontend, button, form, input 🐛 bug Something isn't working labels May 2, 2024
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label May 2, 2024
@graphite-app graphite-app bot requested a review from a team May 2, 2024 15:54
Copy link

graphite-app bot commented May 2, 2024

Graphite Automations

"Add community label" took an action on this PR • (05/02/24)

1 label was added to this PR based on Keith Williams's automation.

"Add consumer team as reviewer" took an action on this PR • (05/02/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

github-actions bot commented May 2, 2024

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@keithwillcode keithwillcode added this to the v4.1 milestone May 8, 2024
@keithwillcode keithwillcode modified the milestones: v4.1, v4.2 May 15, 2024
Copy link
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label May 30, 2024
@PeerRich PeerRich changed the title your name translation fixed fix: your name translation Jun 1, 2024
@github-actions github-actions bot removed the Stale label Jun 2, 2024
@PeerRich PeerRich added the Low priority Created by Linear-GitHub Sync label Jun 3, 2024
@PeerRich PeerRich modified the milestones: v4.2, v4.3 Jun 3, 2024
@malbq
Copy link

malbq commented Jun 3, 2024

I'd like to share another perspective.
I saw the same issue as #14734 when experimenting on a way to translate Booker Atom.
After some debugging, I came to the same conclusion as @smitgol, that the label was overridden by the default value from fieldTypes.ts.

However, the correct label is already translated in:

const { hidden, placeholder, label } = getAndUpdateNormalizedValues(field, t);

My suggestion is to pass it down to the name component factory instead of translating it again: https://github.com/malbq/cal.com/commit/814fa953ae4ea0aa382e0f235647b5fdb28622cb

@dosubot dosubot bot modified the milestone: v4.3 Jun 6, 2024
@dosubot dosubot bot modified the milestones: v4.3, v4.4 Jun 11, 2024
@github-actions github-actions bot added the linear Sync Github Issue from community members to Linear.app label Jun 14, 2024
@hariombalhara hariombalhara force-pushed the form-translation-fix branch 3 times, most recently from b5cdc32 to 8d14d6d Compare June 14, 2024 11:37
Comment on lines -57 to -73
/**
* Get's the field's variantsConfig and if not available, then it will get the default variantsConfig from the fieldTypesConfigMap
*/
export const getVariantsConfig = (field: Pick<z.infer<typeof fieldSchema>, "variantsConfig" | "type">) => {
const fieldVariantsConfig = field.variantsConfig;
const fieldTypeConfig = fieldTypesConfigMap[field.type as keyof typeof fieldTypesConfigMap];

if (!fieldTypeConfig) throw new Error(`Invalid field.type ${field.type}}`);

const defaultVariantsConfig = fieldTypeConfig?.variantsConfig?.defaultValue;
const variantsConfig = fieldVariantsConfig || defaultVariantsConfig;

if (fieldTypeConfig.propsType === "variants" && !variantsConfig) {
throw new Error(`propsType variants must have variantsConfig`);
}
return variantsConfig;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored out in a separate module variantsConfig

Comment on lines 36 to 48
export const getConfig = (field: Pick<z.infer<typeof fieldSchema>, "variantsConfig" | "type">) => {
const fieldVariantsConfig = field.variantsConfig;
const fieldTypeConfig = fieldTypesConfigMap[field.type as keyof typeof fieldTypesConfigMap];

if (!fieldTypeConfig) throw new Error(`Invalid field.type ${field.type}}`);

const defaultVariantsConfig = fieldTypeConfig?.variantsConfig?.defaultValue;
const variantsConfig = fieldVariantsConfig || defaultVariantsConfig;

if (fieldTypeConfig.propsType === "variants" && !variantsConfig) {
throw new Error(`propsType variants must have variantsConfig`);
}
return variantsConfig;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from utils.ts

@hariombalhara hariombalhara force-pushed the form-translation-fix branch from 8d14d6d to 54fe5f8 Compare June 14, 2024 11:41
@@ -333,8 +334,8 @@ export const ComponentForField = ({
}

if (componentConfig.propsType === "variants") {
const variantsConfig = getVariantsConfig(field);
if (!variantsConfig) {
const translatedVariantsConfig = getTranslatedVariantsConfig(field, t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the translatedVariantsConfig directly so that Components don't need to do the translation.

Copy link
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smitgol Thanks for the PR.

I was testing out the PR and found a better approach to fix it. Could you please check the changes out and see if they are acceptable to you

@PeerRich PeerRich enabled auto-merge June 19, 2024 18:41
@PeerRich PeerRich disabled auto-merge June 19, 2024 18:42
@hariombalhara hariombalhara added this pull request to the merge queue Jun 20, 2024
Merged via the queue into calcom:main with commit d0a7eb1 Jun 20, 2024
16 of 23 checks passed
p6l-richard pushed a commit to p6l-richard/cal.com-fork that referenced this pull request Jul 22, 2024
* your name translation fixed

* do translation outside Components as that is needed by all components. Allows the code to stay DRY

---------

Co-authored-by: Hariom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working community Created by Linear-GitHub Sync i18n area: i18n, translations linear Sync Github Issue from community members to Linear.app Low priority Created by Linear-GitHub Sync ui area: UI, frontend, button, form, input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-3883] Event booking - one input label lacks Polish translation
6 participants