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

Ensure FAQ questions are always internationalized #1475

Merged
merged 3 commits into from
May 21, 2020

Conversation

toolness
Copy link
Collaborator

@toolness toolness commented May 21, 2020

While browsing the site with garbled translations active, I noticed that FAQ questions in the preview section weren't garbled:

image

It looks like this was happening because li18n._() was being called in top-level module code in faqs-content.tsx, so I wrapped it all in a function.

While I was at it, I also potentially simplified the sorting of the FAQs with preview content, I'm not sure though. I think code-wise it might be more code, but I think it's also potentially more readable. Feedback welcome @sraby!

@toolness toolness requested a review from sraby May 21, 2020 14:35
Comment on lines 591 to 594
const { previewOptions } = faq;
if (previewOptions) {
FaqsWithPreviewContent.push({ ...faq, previewOptions });
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Super weird that this appears to be the most TypeScript-friendly way to create this array. You'd think we could just say if (faq.previewOptions) FaqsWithPreviewContent.push(faq) but that doesn't work. 😞

Copy link
Member

Choose a reason for hiding this comment

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

Huh, odd!

@toolness toolness changed the title Simplify sorting of FAQs with preview content, maybe. Ensure FAQ questions are always internationalized May 21, 2020
@toolness toolness mentioned this pull request May 21, 2020
33 tasks
Comment on lines +598 to +603
results.sort(
(faq1, faq2) =>
faq1.previewOptions.priorityInPreview -
faq2.previewOptions.priorityInPreview
);

Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is so much simpler to read!

Copy link
Member

@sraby sraby left a comment

Choose a reason for hiding this comment

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

Awesome! This looks good to me, and it's great that you were able to address some of the readability concerns along the way.

@toolness toolness merged commit bb9e095 into master May 21, 2020
@toolness toolness deleted the faqs-with-preview-content-sorting branch May 21, 2020 17:32
@toolness
Copy link
Collaborator Author

Awesome, thanks Sam!

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.

2 participants