Remove t and localizeMessage from components/activity_log_modal #26987
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
mm-cloud-bot
added
the
release-note-none
larkox
approved these changes
May 10, 2024
| const intl = useIntl(); | ||
|
|
||
| let title; | ||
| if (isMessageDescriptor(props.deviceTitle)) { |
There was a problem hiding this comment.
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.
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
harshilsharma63
approved these changes
May 13, 2024
hmhealey
added
4: Reviews Complete
amyblais
added
Changelog/Not Needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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