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: add translations for emails and type error fixes overall #994

Merged
merged 19 commits into from Oct 25, 2021

Conversation

mihaic195
Copy link
Contributor

@mihaic195 mihaic195 commented Oct 19, 2021

  • I still need to translate the dates displayed in emails (day and month), but it's already hard to follow the PR. We can do it in a follow-up PR.

@mihaic195 mihaic195 self-assigned this Oct 19, 2021
@vercel
Copy link

vercel bot commented Oct 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cal/calendso/4fA4zjNPmgGio5YGAPMaP5cEDyY8
✅ Preview: https://calendso-git-i18n-i18n-extract-strings-2-cal.vercel.app

@vercel vercel bot temporarily deployed to Preview October 19, 2021 17:26 Inactive
lib/core/i18n/i18n.utils.ts Outdated Show resolved Hide resolved
lib/core/i18n/i18n.utils.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview October 19, 2021 18:11 Inactive
@zomars zomars self-requested a review October 19, 2021 18:31
@vercel vercel bot temporarily deployed to Preview October 20, 2021 08:59 Inactive
@vercel vercel bot temporarily deployed to Preview October 20, 2021 09:22 Inactive
@mihaic195 mihaic195 changed the title feat: add translations for forgot password email and misc feat: add translations for forgot password/invitation emails and misc Oct 20, 2021
@vercel vercel bot temporarily deployed to Preview October 20, 2021 09:24 Inactive
@vercel vercel bot temporarily deployed to Preview October 20, 2021 09:32 Inactive
Copy link
Contributor

@KATT KATT left a comment

Choose a reason for hiding this comment

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

Looks decent - just rename that function to something more readable

lib/core/i18n/i18n.utils.ts Outdated Show resolved Hide resolved
lib/core/i18n/i18n.utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, got some concerns about a specific error handler so let's discuss.

lib/emails/invitation.ts Outdated Show resolved Hide resolved
lib/emails/invitation.ts Outdated Show resolved Hide resolved
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Code looks good, can't think of more feedback apart from what @emrysal and @KATT have already addressed.

lib/emails/invitation.ts Outdated Show resolved Hide resolved
pages/api/teams/[team]/invite.ts Outdated Show resolved Hide resolved
@@ -157,7 +159,7 @@ export default function ForgotPassword({ csrfToken }) {
);
}

ForgotPassword.getInitialProps = async (context) => {
ForgotPassword.getInitialProps = async (context: GetServerSidePropsContext) => {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't getInitialProps deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it should be changed!

@vercel vercel bot temporarily deployed to Preview October 22, 2021 10:53 Inactive
@mihaic195 mihaic195 changed the title feat: add translations for forgot password/invitation emails and misc feat: add translations for emails and type errors fixes overall Oct 22, 2021
@vercel vercel bot temporarily deployed to Preview October 22, 2021 10:58 Inactive
@vercel vercel bot temporarily deployed to Preview October 22, 2021 11:15 Inactive
@mihaic195 mihaic195 changed the title feat: add translations for emails and type errors fixes overall feat: add translations for emails and type error fixes overall Oct 22, 2021
Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Nice work mate

Comment on lines -11 to -21
constructor(
calEvent: CalendarEvent,
uid: string,
videoCallData: VideoCallData,
additionInformation: AdditionInformation = null
) {
super(calEvent, uid);
this.videoCallData = videoCallData;
this.additionInformation = additionInformation;
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore. Everything is on calEvent 😁

Copy link
Contributor

@KATT KATT left a comment

Choose a reason for hiding this comment

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

Unsure how to test but it looks like an improvement overall!

@@ -38,14 +33,22 @@ export default class CalEventParser {
* Returns a unique identifier for the given calendar event.
*/
public getUid(): string {
return this.maybeUid ?? translator.fromUUID(uuidv5(JSON.stringify(this.calEvent), uuidv5.URL));
return this.calEvent.uid ?? translator.fromUUID(uuidv5(JSON.stringify(this.calEvent), uuidv5.URL));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the calEvent contain dates and stuff? Then this would (wrongly?) give a new unique id if the event is rescheduled

Not sure if it matters at all.

Can use lodash/pick to pick the things you wan to make the uuid-hash based on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved most of the dangling stuff into the calEvent. I've specifically tested this flow to make sure that the uid is only there when rescheduling or updating a booking. Otherwise, it will be a new uid.

@vercel vercel bot temporarily deployed to Preview October 25, 2021 07:48 Inactive
@vercel vercel bot temporarily deployed to Preview October 25, 2021 08:10 Inactive
@vercel vercel bot temporarily deployed to Preview October 25, 2021 08:22 Inactive
@vercel vercel bot temporarily deployed to Preview October 25, 2021 08:25 Inactive
@mihaic195 mihaic195 mentioned this pull request Oct 25, 2021
1 task
Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Was assuming the typescript errors needed fixing first, but without those; this is good to go

@vercel vercel bot temporarily deployed to Preview October 25, 2021 10:24 Inactive
@baileypumfleet baileypumfleet merged commit 8d6fec7 into main Oct 25, 2021
@baileypumfleet baileypumfleet deleted the i18n/i18n-extract-strings-2 branch October 25, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants