-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Remove t and localizeMessage from components/activity_log_modal #26987
Conversation
I wanted to do more to simplify the various values we pass around for icon, title, and text so that we'd just pass around a single "type" value, but since this logic is weird, I gave up on that to focus on localizeMessage and t.
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.
LGTM, just one nit that I am not even sure about 😛
const intl = useIntl(); | ||
|
||
let title; | ||
if (isMessageDescriptor(props.deviceTitle)) { |
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.
0/5 on legibility, but to avoid one import, we can check here typeof props.deviceTitle === 'string'
.
And also 0/5 on legibility, but we could do:
const title = typeof props.deviceTitle === 'string' ? props.deviceTitle : intl.formatMessage(props.deviceTitle);
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 like going with the more descriptive name here than using the typeof
even though they could do the same thing. For the ternary, I ran into some cases where that was causing the line to be a bit long.
I was thinking about making a helper like formatToString(i18n: I18n, value: string | MessageDescriptor): string
though because I feel like I've written this logic a few times lately. Ideally, I think we'd be consistent about what we pass around, but that seems like it'd be helpful in a bunch of places
Summary
I decided to work for a bit on getting rid of these functions from a few more places. As much as I wanted to, I had to keep myself from trying to rewrite all the logic for determining the platform so that, instead of passing a bunch of values around, we would just pass a single "platform type" value
Release Note