MM-56577 Fixing font display issues on thread items#9630
MM-56577 Fixing font display issues on thread items#9630avasconcelos114 wants to merge 5 commits intomainfrom
Conversation
avasconcelos114
added
2: Dev Review
📝 WalkthroughWalkthroughThis change adds comprehensive test coverage for the RemoveMarkdown component and extends it with new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/remove_markdown/index.tsx (1)
146-156: Clarify thefirstparameter semantics.The
firstparameter is used to determine whether to render the paragraph, but its meaning isn't immediately clear. A brief comment would help future maintainers understand that this indicates whether it's the first paragraph following a heading (relevant for the two-line preview).📝 Suggested documentation
- const renderParagraph = useCallback(({children, first}: {children: ReactNode; first: boolean}) => { + // `first` indicates whether this is the first paragraph in the document, + // used to show only the first paragraph in thread previews + const renderParagraph = useCallback(({children, first}: {children: ReactNode; first: boolean}) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/remove_markdown/index.tsx` around lines 146 - 156, The renderParagraph callback's boolean parameter first is unclear; add a short inline comment above the renderParagraph definition explaining that first === true indicates the paragraph is the first paragraph after a heading (used to decide rendering when separateHeading and a two-line preview via numberOfLines/paragraphLines are in effect). Mention renderParagraph, first, separateHeading, paragraphLines, numberOfLines and headingLineCount so maintainers understand the interaction and why only the first paragraph may be rendered.app/components/remove_markdown/index.test.tsx (1)
126-158: Consider strengthening theseparateHeadingtest assertion.The test at lines 144-157 only verifies that "Heading" and "Body" strings exist in the JSON output, but doesn't actually verify that the heading is rendered in a
Viewblock as the test name suggests. Consider checking for structural elements or using a more specific assertion.💡 Proposed enhancement for more robust assertion
it('should render heading in a View block when separateHeading is set', () => { const {toJSON} = renderWithIntl( <RemoveMarkdown baseStyle={baseStyle} numberOfLines={2} separateHeading={true} value={'### Heading\nBody text'} />, ); const json = JSON.stringify(toJSON()); expect(json).toContain('Heading'); expect(json).toContain('Body'); + // Verify View wrapper exists at root level + const tree = toJSON(); + expect(tree?.type).toBe('View'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/remove_markdown/index.test.tsx` around lines 126 - 158, The test for separateHeading is too weak: it only checks that "Heading" and "Body" strings exist but not that the heading is rendered inside a View as intended; update the test for RemoveMarkdown (test suite 'heading block separation') to inspect the rendered tree structure (via toJSON() or the render wrapper APIs) and assert that there is a node with type 'View' (or component name used by RemoveMarkdown for heading blocks) whose children/text equals the heading content, while the body remains in a separate node — locate the assertion around the second it('should render heading in a View block when separateHeading is set') and replace or extend the contains checks with a structural assertion that the heading node is wrapped by a View node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/remove_markdown/index.test.tsx`:
- Around line 126-158: The test for separateHeading is too weak: it only checks
that "Heading" and "Body" strings exist but not that the heading is rendered
inside a View as intended; update the test for RemoveMarkdown (test suite
'heading block separation') to inspect the rendered tree structure (via toJSON()
or the render wrapper APIs) and assert that there is a node with type 'View' (or
component name used by RemoveMarkdown for heading blocks) whose children/text
equals the heading content, while the body remains in a separate node — locate
the assertion around the second it('should render heading in a View block when
separateHeading is set') and replace or extend the contains checks with a
structural assertion that the heading node is wrapped by a View node.
In `@app/components/remove_markdown/index.tsx`:
- Around line 146-156: The renderParagraph callback's boolean parameter first is
unclear; add a short inline comment above the renderParagraph definition
explaining that first === true indicates the paragraph is the first paragraph
after a heading (used to decide rendering when separateHeading and a two-line
preview via numberOfLines/paragraphLines are in effect). Mention
renderParagraph, first, separateHeading, paragraphLines, numberOfLines and
headingLineCount so maintainers understand the interaction and why only the
first paragraph may be rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b367b8c5-d504-46a6-a701-cebed4b1aa02
📒 Files selected for processing (5)
app/components/remove_markdown/index.test.tsxapp/components/remove_markdown/index.tsxapp/screens/global_threads/threads_list/thread/thread.tsxapp/utils/markdown/index.tstypes/global/markdown.ts
|
Let me know whether I should be running each of the E2E Detox tests for this :) |
Coverage Comparison ReportGenerated on March 27, 2026 at 13:54:31 UTC |
matthewbirtch
left a comment
matthewbirtch
left a comment
There was a problem hiding this comment.
thanks @avasconcelos114! LGTM
I think the spacing request between the heading and the body was intended to be an inline space, not a paragraph space. Having said that, I don't mind the heading being on its own line, I just don't think we need the vertical spacing between heading and body. So we can remove the marginBottom on the headingBlock in the thread list context I think.
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
One nitpick, and one question to understand better the PR.
| ); | ||
| }; | ||
|
|
||
| const RemoveMarkdown = ({enableEmoji, enableChannelLink, enableHardBreak, enableSoftBreak, enableCodeSpan, baseStyle, numberOfLines, separateHeading, value}: Props) => { |
There was a problem hiding this comment.
For better visibility on what is added or removed in the future, can you put every property in a different line?
| ); | ||
| }; | ||
|
|
||
| const RemoveMarkdown = ({enableEmoji, enableChannelLink, enableHardBreak, enableSoftBreak, enableCodeSpan, baseStyle, numberOfLines, separateHeading, value}: Props) => { |
There was a problem hiding this comment.
What exactly is supposed to achieve the separateHeading prop?
There was a problem hiding this comment.
My first iteration of this had the heading being rendered in a separate line from the rest if the numberOfLines prop had been passed to the component
it felt strange to have the numberOfLines dictate how the heading was rendered so I added a separateHeading prop that when present, will make sure that there is always a line break between the heading and body of the message
and when missing this prop, it should default to the same behavior as it had prior to this PR, as the RemoveMarkdown component is used by the AnnouncementBanner and ChannelBanner components as well
|
The In this screenshot, the post that contains a heading is the third one with the rabbit emoji |
matthewbirtch
left a comment
matthewbirtch
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114. Looks good from a UI perspective.
|
@larkox gentle reminder to look through this whenever you can :) |
avasconcelos114
added
the
E2E/Run
github-actions
bot
removed
the
E2E/Run
4 participants
Summary
This PR addresses issues with the styling of text as they appear in the thread list.
Also as brought up in the Jira ticket, I also made the changes needed in order to add extra spacing between the heading and body of the message if the heading is present while still keeping the total number of lines displayed at 2
Ticket Link
https://mattermost.atlassian.net/browse/MM-56577
Checklist
E2E/Run(orE2E/Run-iOS/E2E/Run-Androidfor platform-specific runs).Device Information
This PR was tested on:
iPhone 16 Pro (Simulator), iOS 18.3
Screenshots
Release Note