feat: use the headerTitle in the Header/components folder for RoomHeader
#6805
Conversation
WalkthroughAdds Android-specific minHeight to the HeaderContainer; introduces a new optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RoomHeader
participant Pressable
participant HeaderTitle
participant IconAndSubtitle as Icon/Subtitle
Note over RoomHeader,Pressable: User interaction / render flow
RoomHeader->>Pressable: render wrapper (testID, hitSlop)
Pressable->>HeaderTitle: render headerTitle={title ?? ''}, position='left'
Pressable->>IconAndSubtitle: render icon and subtitle elements
Note over HeaderTitle: uses Platform.OS to apply Android alignment via position prop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 5
🧹 Nitpick comments (1)
app/containers/Header/components/HeaderTitle/index.tsx (1)
10-10: Make the default position explicit.The
positionprop is optional but has no explicit default value. Whenundefined, the code implicitly treats it as'center'(sinceposition === 'left'evaluates tofalse). For clarity and maintainability, consider providing an explicit default.Apply this diff:
interface IHeaderTitle { headerTitle?: string | ((props: { children: string; tintColor?: string }) => ReactNode); - position?: 'left' | 'center' | 'right'; + position?: 'left' | 'center' | 'right'; } type HeaderTitleProps = IHeaderTitle & TextProps; -const HeaderTitle = memo(({ headerTitle, position, ...props }: HeaderTitleProps) => { +const HeaderTitle = memo(({ headerTitle, position = 'center', ...props }: HeaderTitleProps) => {
📜 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/RoomHeader/__snapshots__/RoomHeader.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
app/containers/Header/components/HeaderContainer/index.tsx(3 hunks)app/containers/Header/components/HeaderTitle/index.tsx(3 hunks)app/containers/Header/components/HeaderTitle/styles.ts(1 hunks)app/containers/RoomHeader/RoomHeader.tsx(3 hunks)
🔇 Additional comments (4)
app/containers/Header/components/HeaderTitle/styles.ts (1)
17-21: LGTM: Android title style cleanup.The removal of
lineHeightandpaddingVerticalwhile retainingflex: 1is appropriate for the Android title style, allowing the Text to fill available space in its container.app/containers/Header/components/HeaderContainer/index.tsx (1)
20-20: LGTM: Platform-appropriate header heights.The conditional minHeight (56 for Android, 44 for iOS) aligns with Material Design and iOS Human Interface Guidelines for header/app bar heights.
Also applies to: 31-31
app/containers/Header/components/HeaderTitle/index.tsx (1)
13-14: LGTM: Props spreading enhances flexibility.Combining
IHeaderTitlewithTextPropsand spreading additional props to the underlyingTextcomponents is a good enhancement. This allows consumers to pass through standardTextproperties liketestID,accessibilityLabel, etc.Also applies to: 32-32, 46-46
app/containers/RoomHeader/RoomHeader.tsx (1)
3-3: Remove reference to non-existentPlatformPressablecomponent.
PlatformPressabledoes not exist in the codebase. Your current usage ofPressablefromreact-native-gesture-handler(line 172) is straightforward—it only uses basic props (onPress,disabled,hitSlop) without complex gesture handling. Throughout the codebase, the standardPressablefromreact-nativeis used for simple interactions (e.g., UserItem.tsx, EmojiSearchbar.tsx, Chip/index.tsx), whilereact-native-gesture-handler's components are reserved for gesture-heavy interactions (BorderlessButton, RectButton). Consider switching to standardreact-nativePressable for consistency with the existing pattern.Likely an incorrect or invalid review comment.
| color: colors.fontTitlesLabels | ||
| }}> | ||
| color: colors.fontTitlesLabels, | ||
| alignSelf: position === 'left' ? 'flex-start' : 'center' |
There was a problem hiding this comment.
Handle all position values explicitly.
The current implementation only distinguishes between 'left' and everything else (defaulting to 'center'). The 'right' position value is accepted by the type but would incorrectly render as centered. Consider implementing all three position values explicitly.
Apply this diff to handle all position values:
...styles.androidTitle,
color: colors.fontTitlesLabels,
- alignSelf: position === 'left' ? 'flex-start' : 'center'
+ alignSelf: position === 'left' ? 'flex-start' : position === 'right' ? 'flex-end' : 'center'
}}And similarly for the non-Android branch:
...styles.title,
color: colors.fontTitlesLabels,
- alignSelf: position === 'left' ? 'flex-start' : 'center'
+ alignSelf: position === 'left' ? 'flex-start' : position === 'right' ? 'flex-end' : 'center'
}}Also applies to: 44-44
🤖 Prompt for AI Agents
In app/containers/Header/components/HeaderTitle/index.tsx around lines 30 and
44, the style sets alignSelf using a binary check (position === 'left' ?
'flex-start' : 'center') which misrenders position='right'; update both the
Android and non-Android branches to handle all three explicit values ('left' ->
'flex-start', 'center' -> 'center', 'right' -> 'flex-end') (use a switch or
explicit conditional chain) so each position maps to the correct alignSelf.
| justifyContent: 'center', | ||
| alignItems: 'center' |
There was a problem hiding this comment.
Remove ineffective style properties.
justifyContent and alignItems are flex container properties and have no effect on Text components. Text elements are not flex containers unless you explicitly set display: 'flex', which is uncommon in React Native.
Apply this diff to remove the ineffective properties:
title: {
...sharedStyles.textBold,
fontSize: 16,
- justifyContent: 'center',
- alignItems: 'center'
},🤖 Prompt for AI Agents
In app/containers/Header/components/HeaderTitle/styles.ts around lines 14 to 15,
the style object for the Text component includes flex container properties
justifyContent and alignItems which have no effect on Text; remove those two
properties from the styles so the Text style only contains relevant text/layout
properties (e.g., keep textAlign or padding if needed) to clean up dead code.
divyanshu-patil
changed the title
headerTitle in the Header/components folder RoomHeader
divyanshu-patil
changed the title
headerTitle in the Header/components folder RoomHeaderheaderTitle in the Header/components folder for RoomHeader
divyanshu-patil
changed the title
headerTitle in the Header/components folder for RoomHeaderheaderTitle in the Header/components folder for RoomHeader
1 participant
Proposed changes
changes made
RoomHeaderis now using the custom sharedheaderTitletouchableOpacityis now replaced with the PressableheaderTitlehave been modified to avoid clipping of textheaderTitleis aligned left while centered in IOShelp i neededis usingPlatformPressablecausing some tests to fail, so i used thePressablewhich works fine with the header. help me in review for should i strictly usePlatformPressableinstead ofPressableIssue(s)
added feature suggested in: #6255
How to test or reproduce
Screenshots
RoomHeader on Android and IOS with headerTitle from app/containers/Header/components/HeaderTitle/index.tsx
header.title.demo.2.mp4
Types of changes
Checklist
Further comments
onPress()but by looking at the code i think its already implemented, if that mean something else functionality should be implemented as the onPress then add review over thisSummary by CodeRabbit
Improvements
Style