-
Notifications
You must be signed in to change notification settings - Fork 1
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
HelpScout Tasks Search #1120
HelpScout Tasks Search #1120
Conversation
Preview branch generated at https://helpscout-tasks-search.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against d4b2075
|
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 working on this!
When I have PRs without a Jira ticket that are based on HelpScout tickets, I think it can be helpful to put the HelpScout link(s) in the PR description so that if we ever need the context for the change it's linked to the user's bug report or feature request.
greeting = firstName | ||
? t('Good Afternoon, {{ firstName }}.', { firstName }) | ||
: t('Good Afternoon,'); | ||
greeting = t('Good Afternoon,') + (firstName ? ` ${firstName}.` : ''); |
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.
How about removing the string concatenation and after the last else if
doing
if (firstName) {
greeting += ` ${firstName}.`
}
Also, this seems good for now, but if find any more bugs like this, we should probably look into tweaking i18n's interpolation settings.
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'm sure there are many more bugs because of the interpolation settings. My work around in the past has been to use a component where I can set interpolation: { escapeValue: false },
? t('Search subject, tags, contact name, comments') | ||
: t('Search name, phone, email, partner #') |
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.
What do you think about this tweak?
? t('Search subject, tags, contact name, comments') | |
: t('Search name, phone, email, partner #') | |
? t('Search by subject, tags, contact name, or comments') | |
: t('Search by name, phone, email, or partner #') |
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.
✅
Description
https://secure.helpscout.net/conversation/2726128388/1238886?folderId=7669074
Checklist: