fix: custom status text wrap in drawer#6778
Conversation
|
Warning Rate limit exceeded@diegolmello has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
|
@Rohit3523 @diegolmello can you review my PR and let me any further modifications needed in this |
|
Hey @Rohit3523 , one of the tests failed in the CI run. Could you please help me check what might be causing it? |
Update the storybook snap using |
ok, I will check and update it |
|
@Rohit3523 @diegolmello can anyone review the PR and let me know any changes required |
|
I think it will also truncate other list items, can you please test this case |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/List/ListItem.tsx (1)
68-68: Refine theellipsizeModetype for better type safety.The
as anycast on line 77 indicates a type mismatch. React Native'sTextcomponent expectsellipsizeModeto be'head' | 'middle' | 'tail' | 'clip', but it's currently typed asstringinIListItemContent(line 110).Apply this diff to improve type safety:
In IListItemContent interface (line 110):
numberOfLines?: number; - ellipsizeMode?: string; + ellipsizeMode?: 'head' | 'middle' | 'tail' | 'clip';In ListTitle component (line 77):
<Text numberOfLines={numberOfLines} - ellipsizeMode={ellipsizeMode as any} + ellipsizeMode={ellipsizeMode} style={[styles.title, styleTitle, { color: color || colors.fontDefault }]}>Also applies to: 75-78
📜 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/List/ListItem.tsx(4 hunks)app/views/SidebarView/components/CustomStatus.tsx(1 hunks)
🔇 Additional comments (3)
app/views/SidebarView/components/CustomStatus.tsx (1)
75-76: LGTM! Fix correctly addresses the issue.The addition of
numberOfLines={1}andellipsizeMode='tail'properly constrains the custom status text to a single line with ellipsis truncation, resolving the layout issue reported in #6776.app/containers/List/ListItem.tsx (2)
65-66: LGTM! Interface correctly extended.The interface properly picks the new
numberOfLinesandellipsizeModeprops fromIListItemContent.
132-134: LGTM! Props correctly threaded through the component.The
numberOfLinesandellipsizeModeprops are properly destructured and forwarded toListTitle. The optional nature of these props ensures backward compatibility with existing usage.Also applies to: 176-185
|
@Rohit3523 made changes such that it only truncates particular status list item |
|
Can you please update your branch with develop and add a before screenshot in the PR description |
|
@Rohit3523 I have updated the develop branch and also added a screenshot for before in the PR description |
|
@Rohit3523 @diegolmello @OtavioStasiak can anyone review my PR |
|
Hey, sorry for the delay. In |
|
You can remove the other props and keep text and new prop you have added, also after this run |
|
ok sure |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/List/List.stories.tsx (1)
24-24: AddtranslateTitle={false}for consistency.The
longTextvariable contains literal text rather than a translation key. Following the pattern established elsewhere in this file (lines 22, 34, 38), you should includetranslateTitle={false}when usinglongTextas the title.Apply this diff:
-<List.Item title={longText} numberOfLines={1} /> +<List.Item title={longText} numberOfLines={1} translateTitle={false} />
📜 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/List/__snapshots__/List.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
app/containers/List/List.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/List/List.stories.tsx (1)
.rnstorybook/utils.ts (1)
longText(1-2)
|
Sorry, I just noticed it is important to have translateTitle={false} because title will translate if we don't pass this props ;-; |
|
ok I will update it |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/List/List.stories.tsx (1)
24-25: LGTM! Story correctly demonstrates truncation.The added
List.Itemeffectively demonstrates the single-line truncation behavior withnumberOfLines={1}. The implementation follows the reviewer's guidance and aligns with the PR objectives.Optional: Consider adding
ellipsizeModefor completeness.Based on the AI summary, the actual implementation in
CustomStatus.tsxuses bothnumberOfLines={1}andellipsizeMode='tail'. For consistency and to fully document the truncation behavior in the story, you could optionally addellipsizeMode='tail':-<List.Item title={longText} subtitle={longText} translateTitle={false} translateSubtitle={false} numberOfLines={1} /> +<List.Item title={longText} subtitle={longText} translateTitle={false} translateSubtitle={false} numberOfLines={1} ellipsizeMode='tail' />This would make the story a complete demonstration of the truncation implementation used in the actual feature.
📜 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/List/__snapshots__/List.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
app/containers/List/List.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/List/List.stories.tsx (1)
.rnstorybook/utils.ts (1)
longText(1-2)
|
Umm, ellipsizeMode is missing ;-; |
|
added the the ellipsizeMode prop |
|
Looking good to merge |
|
Thanks a lot for the review and your support |
|
can you approve this PR and merge it |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/containers/List/ListItem.tsx (2)
68-87: Consider addingellipsizeModefor explicit truncation behavior.The implementation correctly forwards
numberOfLinesto theTextcomponent. While React Native defaults toellipsizeMode="tail"whennumberOfLinesis set, explicitly specifying it improves code clarity and matches the story usage pattern requested by the reviewer.Apply this diff:
const ListTitle = ({ title, color, styleTitle, translateTitle, numberOfLines }: IListTitle) => { 'use memo'; const { colors } = useTheme(); switch (typeof title) { case 'string': return ( <Text numberOfLines={numberOfLines} + ellipsizeMode={numberOfLines ? 'tail' : undefined} style={[styles.title, styleTitle, { color: color || colors.fontDefault }]}> {translateTitle && title ? I18n.t(title) : title} </Text> ); case 'function': return title(); default: return null; } };Note: Function-based titles (case 'function') will not respect
numberOfLinessince they return custom JSX. This is likely intentional but worth documenting if it affects your use case.
130-133: Minor: Remove trailing whitespace.There's an unnecessary empty line after the
numberOfLinesparameter.Apply this diff:
accessibilityLabel, numberOfLines, - }: IListItemContent) => {
📜 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/List/__snapshots__/List.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
app/containers/List/List.stories.tsx(1 hunks)app/containers/List/ListItem.tsx(4 hunks)app/views/SidebarView/components/CustomStatus.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/SidebarView/components/CustomStatus.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/List/List.stories.tsx (1)
.rnstorybook/utils.ts (1)
longText(1-2)
app/containers/List/ListItem.tsx (2)
app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
colors(280-302)
🔇 Additional comments (3)
app/containers/List/ListItem.tsx (3)
65-66: LGTM! Clean interface extension.The
numberOfLinesaddition toIListTitleproperly maintains type consistency withIListItemContentusing thePickutility type.
108-108: LGTM! Proper optional prop.The
numberOfLinesprop is correctly typed as optional, allowing components to opt into truncation behavior as needed.
174-182: LGTM! Proper prop forwarding.The
numberOfLinesprop is correctly forwarded fromContenttoListTitle, completing the prop threading fromListItem→Content→ListTitle→Text.Note: The subtitle on line 188 has
numberOfLines={1}hardcoded, meaning subtitles are always single-line regardless of thenumberOfLinesprop. This appears intentional and aligns with the PR objective of keeping the sidebar layout compact.
|
@Yaddalapalli-Charan-Kumar-Naidu merged, thanks for your contribution! |
|
@diegolmello Thank you, I appreciate the review and guidance. Excited to continue contributing |
Proposed changes
This PR fixes a UI issue where the Custom Status text in the sidebar would overflow or wrap into multiple lines, causing layout misalignment.
The fix ensures that the status message is truncated with ellipsis (
...) when it exceeds a single line, maintaining a clean and consistent sidebar layout across all devices and themes.Issue(s)
Closes #6776
https://rocketchat.atlassian.net/browse/COMM-95
How to test or reproduce
...at the end.Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Bug Fixes
New Features
Documentation / Demo
✏️ Tip: You can customize this high-level summary in your review settings.