feat(text-group): add TextGroupList feature #81#625
Open
airikej wants to merge 2 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds ChangesTextGroupList Subcomponent
Sequence DiagramsequenceDiagram
participant Consumer
participant TextGroup
participant TextGroupBase
participant TextGroupList
participant Label
participant DOM
Consumer->>TextGroup: import { TextGroup }
TextGroup->>TextGroupBase: Object.assign reference
TextGroup->>TextGroupList: List subcomponent attached
Consumer->>TextGroupList: render with items array
TextGroupList->>TextGroupList: resolve breakpoint props
TextGroupList->>Label: wrap string labels
Label->>DOM: render wrapped label
TextGroupList->>DOM: render dt/dd pairs in dl
TextGroupList->>DOM: apply --label-width CSS variable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 (2)
src/tedi/components/content/text-group/text-group-list/text-group-list.tsx (2)
122-122: 💤 Low valueConsider adding an optional identifier to TextGroupListItem for stable keys.
Using array index as the
keyprop works for static lists but can cause rendering issues if items are reordered, filtered, or dynamically updated. Consider adding an optionalidorkeyfield to theTextGroupListIteminterface.Optional enhancement
export interface TextGroupListItem { + /** + * Optional unique identifier for this item. Used as React key if provided. + */ + id?: string | number; /** * Label rendered as the `<dt>` for this row.Then in the component:
- <div key={index} className={styles['tedi-text-group__row']} style={rowStyle}> + <div key={item.id ?? index} className={styles['tedi-text-group__row']} style={rowStyle}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/text-group/text-group-list/text-group-list.tsx` at line 122, The list currently uses the array index as the React key which can break rendering when items change; update the TextGroupListItem interface to include an optional unique identifier (e.g., id or uid), accept that field in the component props for TextGroupList (and any parent types), and switch the key on the rendered div in TextGroupList (the element with className styles['tedi-text-group__row']) from index to item.id || index so existing callers remain compatible while enabling stable keys when provided.
82-86: ⚡ Quick winExtract duplicated helper functions to a shared utility file.
The
renderLabelContentandresolveLabelWidthfunctions are identical to those intext-group.tsx(lines 60-64). This code duplication could lead to maintenance issues if one is updated without the other.♻️ Refactor suggestion
Create a shared utilities file (e.g.,
text-group/text-group.utils.ts) and export these helpers:import React from 'react'; import { Label } from '../label/label'; export const renderLabelContent = (label: React.ReactNode): React.ReactNode => typeof label === 'string' ? <Label>{label}</Label> : label; export const resolveLabelWidth = (labelWidth: string | number): string => typeof labelWidth === 'number' ? `${labelWidth}%` : labelWidth;Then import from both
text-group.tsxandtext-group-list.tsx:+import { renderLabelContent, resolveLabelWidth } from './text-group.utils'; - -const renderLabelContent = (label: React.ReactNode): React.ReactNode => - typeof label === 'string' ? <Label>{label}</Label> : label; - -const resolveLabelWidth = (labelWidth: string | number): string => - typeof labelWidth === 'number' ? `${labelWidth}%` : labelWidth;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/text-group/text-group-list/text-group-list.tsx` around lines 82 - 86, Extract the duplicated helpers renderLabelContent and resolveLabelWidth into a shared utility module (e.g., create text-group.utils.ts exporting renderLabelContent and resolveLabelWidth), ensure the utility imports React and Label for renderLabelContent, then replace the local definitions in both text-group.tsx and text-group-list.tsx with imports from the new text-group.utils.ts; verify typings/signatures remain identical and update any import paths accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tedi/components/content/text-group/text-group.spec.tsx`:
- Around line 138-235: The tests for TextGroup.List are unstable because they
rely on responsive behavior; mock the useBreakpointProps hook at the top of this
spec so the List renders deterministically — import or jest.mock the module that
exports useBreakpointProps (the hook used by TextGroup.List) and have it return
a simple passthrough implementation (props => props) or a fixed object for the
cases under test before any renders, then run the existing assertions; ensure
the mock is applied in this spec file (affecting TextGroup.List) so class and
style assertions are environment-independent.
- Around line 150-161: Replace raw DOM selectors in the test (currently using
container.querySelectorAll('dl'), 'dt', 'dd' and mapping textContent into
labels/values) with RTL semantic queries: use within(container) (or screen) and
getAllByRole('list') to assert there is one list and it has classes
'tedi-text-group' and 'tedi-text-group--list', then use getAllByRole/getByRole
or getAllByText/getByText to find the term labels and definition values (e.g.,
replace the labels mapping with within(list).getAllByText(...) or
within(list).getAllByRole(...) and similarly for values), and update the expect
assertions to compare the returned element textContent via .textContent or .text
to the expected ['Patient','Address','Vaccine'] and ['Mari Maasikas','Tulbi tn
4, Tallinn','COVID-19 mRNA']; do the same replacement for the other test blocks
referenced (lines ~176-179, 192-195, 209-213, 225-229, 233-234).
---
Nitpick comments:
In `@src/tedi/components/content/text-group/text-group-list/text-group-list.tsx`:
- Line 122: The list currently uses the array index as the React key which can
break rendering when items change; update the TextGroupListItem interface to
include an optional unique identifier (e.g., id or uid), accept that field in
the component props for TextGroupList (and any parent types), and switch the key
on the rendered div in TextGroupList (the element with className
styles['tedi-text-group__row']) from index to item.id || index so existing
callers remain compatible while enabling stable keys when provided.
- Around line 82-86: Extract the duplicated helpers renderLabelContent and
resolveLabelWidth into a shared utility module (e.g., create text-group.utils.ts
exporting renderLabelContent and resolveLabelWidth), ensure the utility imports
React and Label for renderLabelContent, then replace the local definitions in
both text-group.tsx and text-group-list.tsx with imports from the new
text-group.utils.ts; verify typings/signatures remain identical and update any
import paths accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89194050-9f9f-4e9e-b2c6-134753877c36
📒 Files selected for processing (7)
src/tedi/components/content/text-group/index.tssrc/tedi/components/content/text-group/text-group-list/text-group-list.tsxsrc/tedi/components/content/text-group/text-group.module.scsssrc/tedi/components/content/text-group/text-group.spec.tsxsrc/tedi/components/content/text-group/text-group.stories.tsxsrc/tedi/components/content/text-group/text-group.tsxsrc/tedi/index.ts
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 by CodeRabbit