refactor: create reusable emoji component (issue #6535) #6813
Conversation
|
|
WalkthroughAdds a new reusable Emoji component and index re-export, updates EmojiPicker and Markdown emoji renderers to use the shared component, and adds unit tests covering both unicode and custom emoji rendering. Sizing and shortname-to-unicode conversion are centralized with fontScale-aware logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Picker as EmojiPicker
participant Markdown as MarkdownEmoji
participant Shared as Emoji (shared)
participant Custom as CustomEmoji
participant Theme as ThemeProvider
Picker->>Shared: render(literal=":{emoji}:" / customEmoji)
Markdown->>Shared: render(literal from getEmojiToken(...))
alt customEmoji provided or resolved
Shared->>Custom: render(customEmoji, size = isBig ? big*fontScale : small*fontScale)
else
Shared->>Shared: formatShortnameToUnicode(literal) → unicode
Shared->>Theme: read colors & fontScale/fontScaleLimited
Shared->>Shared: render Unicode text (apply big/avatar styles)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/containers/Emoji/Emoji.tsx (1)
27-57: Optional: deduplicate custom emoji sizing logicThe size objects for
customEmojiandfoundCustomEmojibranches are identical. You can DRY this up for readability:- if (customEmoji) { - const customEmojiSize = { - width: 15 * fontScale, - height: 15 * fontScale - }; - const customEmojiBigSize = { - width: 30 * fontScale, - height: 30 * fontScale - }; - return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={customEmoji} />; - } + const customEmojiSize = { + width: 15 * fontScale, + height: 15 * fontScale + }; + const customEmojiBigSize = { + width: 30 * fontScale, + height: 30 * fontScale + }; + + if (customEmoji) { + return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={customEmoji} />; + } … - if (foundCustomEmoji) { - const customEmojiSize = { - width: 15 * fontScale, - height: 15 * fontScale - }; - const customEmojiBigSize = { - width: 30 * fontScale, - height: 30 * fontScale - }; - return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={foundCustomEmoji} />; - } + if (foundCustomEmoji) { + return <CustomEmoji style={[isBigEmoji ? customEmojiBigSize : customEmojiSize, style]} emoji={foundCustomEmoji} />; + }This keeps the behavior identical while simplifying the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/containers/Emoji/Emoji.tsx(1 hunks)app/containers/Emoji/index.ts(1 hunks)app/containers/EmojiPicker/Emoji.tsx(1 hunks)app/containers/markdown/components/emoji/Emoji.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/EmojiPicker/Emoji.tsx (1)
app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
app/containers/markdown/components/emoji/Emoji.tsx (4)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/containers/markdown/Markdown.stories.tsx (1)
Emoji(94-101)app/containers/markdown/components/emoji/index.ts (1)
Emoji(4-4)app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
🪛 ESLint
app/containers/markdown/components/emoji/Emoji.tsx
[error] 25-25: 'index' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
app/containers/Emoji/Emoji.tsx
[error] 2-2: Imports "StyleProp" and "TextStyle" are only used as type.
(@typescript-eslint/consistent-type-imports)
[error] 25-25: 'convertAsciiEmoji' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (3)
app/containers/Emoji/index.ts (1)
1-1: Barrel export looks goodRe-exporting the default Emoji as a named export is clean and keeps import sites consistent.
app/containers/EmojiPicker/Emoji.tsx (1)
5-11: Confirm expected shape ofemojistring for picker itemsHere you assume string emojis are shortcodes without colons and wrap them as
:${emoji}:. If any caller already passes a colon-wrapped shortcode (e.g.:smile:), this would produce::smile::and fail lookup/conversion. Please double‑check the upstreamIEmojistring representation for picker items and adjust (or guard) if mixed formats are possible.app/containers/markdown/components/emoji/Emoji.tsx (1)
15-30: Clean up unusedindexprop in component signature
indexis part ofIEmojiProps(line 11 of the interface) but not used inside the component, causing the@typescript-eslint/no-unused-varswarning. Removing it from the destructuring while keeping it in the interface is the correct approach:-const Emoji = ({ block, isBigEmoji, style, index, isAvatar }: IEmojiProps) => { +const Emoji = ({ block, isBigEmoji, style, isAvatar }: IEmojiProps) => {Call sites can still pass
index, but the linter will no longer complain about the unused variable.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/containers/Emoji/Emoji.tsx (1)
26-26: Remove unusedconvertAsciiEmojiselector.This variable is declared but never used in the component. The past review already flagged this issue, and it remains unresolved.
- const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji);
🧹 Nitpick comments (2)
app/containers/Emoji/Emoji.tsx (2)
17-18: Consider stronger typing for custom emoji properties.The
anytypes forgetCustomEmojireturn value andcustomEmojireduce type safety. Consider using a more specific type based on the CustomEmoji model fromapp/lib/database/model/CustomEmoji.jsor defining a shared interface.For example:
import type { CustomEmoji } from '../../lib/database/model/CustomEmoji'; interface ISharedEmojiProps { // ... getCustomEmoji?: (name: string) => CustomEmoji | undefined; customEmoji?: CustomEmoji; }
21-83: Consider extracting magic numbers to named constants.The component uses several magic numbers for sizing (15, 30 for emoji dimensions; 16, 22, 30, 43 for font sizes and line heights). Extracting these to named constants would improve code maintainability and make the sizing intent more explicit.
For example:
const EMOJI_SIZE = { SMALL: 15, BIG: 30 }; const TEXT_STYLE = { SMALL: { fontSize: 16, lineHeight: 22 }, BIG: { fontSize: 30, lineHeight: 43 } };Then use these constants throughout the component instead of literal numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/containers/Emoji/Emoji.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/Emoji/Emoji.tsx (7)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)app/lib/constants/colors.ts (1)
colors(280-302)
🪛 ESLint
app/containers/Emoji/Emoji.tsx
[error] 26-26: 'convertAsciiEmoji' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
app/containers/Emoji/Emoji.tsx (1)
2-3: Type-only imports correctly applied.The type-only imports for
StylePropandTextStylehave been correctly implemented, resolving the previous ESLint warning.
… textAlignVertical
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/containers/Emoji/Emoji.tsx (1)
8-9: Remove unusedconvertAsciiEmojiselector and related imports
convertAsciiEmojiis selected from the store but never used, and the only consumers ofuseAppSelector/getUserSelectorin this file are for that value. This will keep tripping@typescript-eslint/no-unused-varsand unused import rules; you can safely delete the selector line and the two imports.-import { useAppSelector } from '../../lib/hooks/useAppSelector'; -import { getUserSelector } from '../../selectors/login'; @@ - const convertAsciiEmoji = useAppSelector(state => getUserSelector(state)?.settings?.preferences?.convertAsciiEmoji);ASCII emoji handling is already encapsulated in
formatShortnameToUnicode, so behavior shouldn’t change. Based on static analysis hints and prior review…Also applies to: 26-26
🧹 Nitpick comments (1)
app/containers/Emoji/Emoji.tsx (1)
12-19: Consider tighteningcustomEmoji/getCustomEmojitypingRight now both
customEmojiand the return type ofgetCustomEmojiareany. If feasible, wiring these to a shared CustomEmoji type (e.g., the WatermelonCustomEmojimodel or a dedicatedICustomEmojiinterface) would give you better autocomplete and catch mismatches at compile time, especially as this component becomes the central emoji renderer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/containers/Emoji/Emoji.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/Emoji/Emoji.tsx (7)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)app/lib/constants/colors.ts (1)
colors(280-302)
🪛 ESLint
app/containers/Emoji/Emoji.tsx
[error] 26-26: 'convertAsciiEmoji' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
app/containers/Emoji/Emoji.tsx (1)
21-71: Emoji rendering flow and sizing look consistent and reusableThe control flow (explicit
customEmoji→ literal-based lookup viagetCustomEmoji→ UnicodeTextfallback) and the shared custom emoji size definitions align well with the PR goal of centralizing emoji rendering. The use offontScale/fontScaleLimitedand theme colors is coherent and should keep existing visuals stable while de-duplicating logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/containers/Emoji/__tests__/Emoji.test.tsx (1)
27-37: Emoji tests correctly cover main rendering pathsThe tests validate both standard emoji rendering via
literaland custom emoji rendering viacustomEmojiwith appropriate mocks. If you expand behavior later, consider adding cases forisBigEmoji,isAvatar, and thegetCustomEmoji(literal)path, but not required for this refactor.app/containers/Emoji/Emoji.tsx (1)
12-19: Consider strengthening types for custom emoji props
getCustomEmojiandcustomEmojiare typed asany, which loses type safety for this shared component. If you have a shared type for custom emojis (e.g., the model behindCustomEmoji), consider wiring it in here for better tooling and safer refactors in callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
app/containers/Emoji/Emoji.tsx(1 hunks)app/containers/Emoji/__tests__/Emoji.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/Emoji/Emoji.tsx (5)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)app/lib/constants/colors.ts (1)
colors(280-302)
🪛 ESLint
app/containers/Emoji/Emoji.tsx
[error] 8-8: 'useAppSelector' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 9-9: 'getUserSelector' is defined but never used.
(@typescript-eslint/no-unused-vars)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/containers/Emoji/Emoji.tsx (1)
10-16: Consider tighteningstyletyping to avoid the ImageStyle cast
styleis declared asStyleProp<TextStyle>but then cast toStyleProp<ImageStyle>when passed intoCustomEmoji. This works at runtime, but it slightly undermines TypeScript’s guarantees and may be confusing to readers. If this component continues to serve both text and image use cases, consider either:
- Splitting props (e.g.,
textStylefor the<Text>path andimageStyleforCustomEmoji), or- Using a more generic style type that doesn’t require the
as StyleProp<ImageStyle>cast.Not urgent, but it would make the API and intent clearer.
Also applies to: 36-49
app/containers/markdown/components/emoji/Emoji.tsx (1)
15-23: ValidategetEmojiTokenagainst allEmojiPropsvariants and add tests
getEmojiTokenappears to cover unicode (unicodein block), avatar (usingblock.value?.value), and the shortCode/value fallback, but its correctness is tightly coupled to how@rocket.chat/message-parserstructuresEmojiProps. It would be good to:
- Double‑check that these are all the relevant shapes for emoji tokens (unicode, custom with
shortCode, custom withoutshortCode, avatar), and- Add unit tests (or story cases) for each to ensure the produced
literalis always whatSharedEmojiandformatShortnameToUnicodeexpect.That should protect this refactor from subtle regressions if the parser evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/containers/Emoji/Emoji.tsx(1 hunks)app/containers/markdown/components/emoji/Emoji.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/markdown/components/emoji/Emoji.tsx (4)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/containers/markdown/Markdown.stories.tsx (1)
Emoji(94-101)app/containers/markdown/components/emoji/index.ts (1)
Emoji(4-4)app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
app/containers/Emoji/Emoji.tsx (5)
app/containers/EmojiPicker/Emoji.tsx (1)
Emoji(7-12)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useResponsiveLayout/useResponsiveLayout.tsx (1)
useResponsiveLayout(41-45)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)app/lib/constants/colors.ts (1)
colors(280-302)
🔇 Additional comments (2)
app/containers/Emoji/Emoji.tsx (1)
19-50: Shared Emoji rendering and sizing logic look solidThe control flow between
customEmoji,literal→emojiUnicode, andgetCustomEmojilookup is clear and null-safe, and the centralizedcustomEmojiSize/customEmojiBigSizedefinitions keep sizing consistent across both direct and looked-up custom emojis. The use offontScalevsfontScaleLimitedfor images vs text also aligns with the intent to respect accessibility scaling without duplicating logic elsewhere.app/containers/markdown/components/emoji/Emoji.tsx (1)
25-30: Delegation toSharedEmojikeeps Markdown emoji rendering consistentWiring
literal,isBigEmoji,style,isAvatar, andgetCustomEmojistraight intoSharedEmojicleanly centralizes the rendering logic, so Markdown emojis now share the same sizing and custom-emoji behavior as the picker and other consumers. This matches the PR’s reuse objective without changing the public Markdown emoji API.
2 participants
Proposed changes
Refactored the Emoji component to be reusable across the application.
EmojiPickerto use the new shared component.Issue(s)
Closes #6535
How to test or reproduce
:smile:) and custom emojis.Screenshots
N/A (Refactor with no visual changes intended)
Types of changes
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.