refactor(BlockQuote): forward ref support #1408
Conversation
WalkthroughThe BlockQuote component is refactored to use React.forwardRef with updated props based on blockquote’s native props plus custom fields. Data-attribute construction is reorganized, a new customRootClass prop is added, and default export is introduced. Tests are added for ref forwarding and data-attribute assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant App as App Component
participant BlockQuote as BlockQuote (forwardRef)
participant DOM as <blockquote>
Dev->>App: Render <BlockQuote ref={qRef} variant size ...>
App->>BlockQuote: Props + ref
rect rgba(213,232,253,0.45)
note right of BlockQuote: Build className/customRootClass and data-* (variant/size/color)
BlockQuote->>DOM: Render blockquote {...props} data-* ref
end
DOM-->>BlockQuote: element handle
BlockQuote-->>App: forwarded ref current = HTMLQuoteElement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
kotAPI
deleted the
kotapi/refactor-blockquote-component-for-forwardref
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/ui/BlockQuote/BlockQuote.tsx (1)
16-16: Drop unnecessary defaulting to empty strings.Optional props don’t need
='';clsxand conditionals handleundefinedfine. Slightly cleaner and avoids accidental truthiness bugs.- ({ children, customRootClass = '', className = '', color = '', variant = '', size = '', ...props }, ref) => { + ({ children, customRootClass = '', className, color, variant, size, ...props }, ref) => {src/components/ui/BlockQuote/tests/BlockQuote.test.tsx (2)
35-39: Strengthen the ref test with an instance check.const ref = React.createRef<HTMLQuoteElement>(); render(<BlockQuote ref={ref}>BlockQuote</BlockQuote>); expect(ref.current).not.toBeNull(); + expect(ref.current).toBeInstanceOf(HTMLQuoteElement);
41-46: Add a negative case for absent data attributes.Guard against regressions by asserting variant/size attrs are not rendered when props are omitted.
Proposed test (new block):
test('does not render variant/size data attributes when not provided', () => { render(<BlockQuote>BlockQuote</BlockQuote>); const el = screen.getByText('BlockQuote'); expect(el).not.toHaveAttribute('data-block-quote-variant'); expect(el).not.toHaveAttribute('data-block-quote-size'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/components/ui/BlockQuote/BlockQuote.tsx(1 hunks)src/components/ui/BlockQuote/tests/BlockQuote.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/components/ui/BlockQuote/BlockQuote.tsx (3)
15-39: ForwardRef conversion and prop typing look solid.Ref is correctly forwarded to the blockquote; prop surface aligns with native props + custom fields.
34-37: Confirm precedence: internal data-attrs vs consumer-supplied ones.You spread
...propsbefore...data_attributes, so library-generated attributes overwrite any user-provideddata-*. If that’s intentional, keep it. If you prefer consumer override, flip the order.
34-37: Verifyclsximport style.You’re calling
clsx(...)but the package exposes a default export. Ensure the import matches your bundler/TS config; default import is the safe choice.Recommended import (outside this hunk):
import clsx from 'clsx';src/components/ui/BlockQuote/tests/BlockQuote.test.tsx (1)
35-39: Ref forwarding test reads well.
| const data_attributes: Record<string, string> = {}; | ||
|
|
||
| if (size) { | ||
| data_attributes['data-block-quote-size'] = size; | ||
| } | ||
| if (variant) { | ||
| data_attributes['data-block-quote-variant'] = variant; | ||
| } | ||
|
|
||
| if (color) { | ||
| data_attributes['data-rad-ui-accent-color'] = color; | ||
| } | ||
| if (size) { | ||
| data_attributes['data-block-quote-size'] = size; | ||
| } | ||
|
|
||
| return <blockquote className={clsx(rootClass, className)} {...props} {...data_attributes}> {children}</blockquote>; | ||
| }; | ||
| if (color) { | ||
| data_attributes['data-rad-ui-accent-color'] = color; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten type for data attributes to avoid TS JSX spread errors.
Spreading Record<string, string> into JSX can fail under strict React types because it allows non-data-* keys. Constrain to data-* keys.
- const data_attributes: Record<string, string> = {};
+ const data_attributes: Partial<Record<`data-${string}`, string | number | boolean | undefined>> = {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data_attributes: Record<string, string> = {}; | |
| if (size) { | |
| data_attributes['data-block-quote-size'] = size; | |
| } | |
| if (variant) { | |
| data_attributes['data-block-quote-variant'] = variant; | |
| } | |
| if (color) { | |
| data_attributes['data-rad-ui-accent-color'] = color; | |
| } | |
| if (size) { | |
| data_attributes['data-block-quote-size'] = size; | |
| } | |
| return <blockquote className={clsx(rootClass, className)} {...props} {...data_attributes}> {children}</blockquote>; | |
| }; | |
| if (color) { | |
| data_attributes['data-rad-ui-accent-color'] = color; | |
| } | |
| const data_attributes: Partial<Record<`data-${string}`, string | number | boolean | undefined>> = {}; | |
| if (variant) { | |
| data_attributes['data-block-quote-variant'] = variant; | |
| } | |
| if (size) { | |
| data_attributes['data-block-quote-size'] = size; | |
| } | |
| if (color) { | |
| data_attributes['data-rad-ui-accent-color'] = color; | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/BlockQuote/BlockQuote.tsx around lines 19 to 31, the
data_attributes variable is typed as Record<string, string>, which is too broad
for JSX spread; replace its type with Partial<Record<`data-${string}`, string>>
(or {[k: `data-${string}`]: string} | undefined) so only data-* keys are
allowed, keep the existing conditional assignments (data-block-quote-variant,
data-block-quote-size, data-rad-ui-accent-color) and then spread data_attributes
into the JSX; this tightens the type and prevents TS JSX spread errors.
Summary
Testing
npm test src/components/ui/BlockQuote/tests/BlockQuote.test.tsxSummary by CodeRabbit
New Features
Refactor
Tests