as prop for text component
#609
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
src/components/ui/Text/stories/Text.stories.js (2)
11-11: Consider using template literal with proper indentationThe multiline text content might render with unintended whitespace. Consider using proper template literal formatting:
- <Text {...args}> {`I'm not a monkey -I will not dance even if the beat's funky`} </Text> + <Text {...args}> + {`I'm not a monkey + I will not dance even if the beat's funky`} + </Text>
24-29: Consider adding moreasprop variantsThe story only demonstrates the
divvariant. Consider adding more stories to showcase other supported tags (span,label).export const AllVariants = { render: (args) => ( <> <Text {...args} as="div">As div</Text> <Text {...args} as="span">As span</Text> <Text {...args} as="label">As label</Text> <Text {...args}>Default (p)</Text> </> ), args: { className: 'text-gray-950' } };src/components/ui/Text/Text.tsx (2)
10-27: Simplify componentList structureThe
labelproperty appears unused. Consider simplifying the array to just contain the tags:-const componentList = [ - { - label: 'DIV', - tag: 'div' - }, - { - label: 'SPAN', - tag: 'span' - }, - { - label: 'P', - tag: 'p' - }, - { - label: 'LABEL', - tag: 'label' - }, -] +const VALID_TAGS = ['div', 'span', 'p', 'label'] as const;This would also simplify the type safety of the
asprop.🧰 Tools
🪛 eslint
[error] 26-26: Unexpected trailing comma.
(comma-dangle)
35-35: Improve default prop handlingThe default value for
asshould be explicit:-const Text = ({ children, customRootClass = '', className = '', as=undefined, ...props }: TextProps) => { +const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => {🧰 Tools
🪛 eslint
[error] 35-35: Operator '=' must be spaced.
(space-infix-ops)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Text/Text.tsx(1 hunks)src/components/ui/Text/stories/Text.stories.js(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Text/Text.tsx
[error] 26-26: Unexpected trailing comma.
(comma-dangle)
[error] 35-35: Operator '=' must be spaced.
(space-infix-ops)
[error] 38-38: Operator '=' must be spaced.
(space-infix-ops)
🔇 Additional comments (1)
src/components/ui/Text/stories/Text.stories.js (1)
18-22: LGTM! Clear demonstration of default behavior
The story effectively shows the Text component's default rendering as a <p> tag.
| children: React.ReactNode; | ||
| customRootClass?: string; | ||
| className?: string; | ||
| as?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety for the as prop
The current type allows any string for the as prop. Consider using a union type of valid tags:
+type ValidTag = typeof VALID_TAGS[number];
export type TextProps = {
children: React.ReactNode;
customRootClass?: string;
className?: string;
- as?: string;
+ as?: ValidTag;
-} & React.ComponentProps<'p'>;
+} & React.ComponentProps<ValidTag>;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
You could just limit the type to
as?: 'div' | 'span' | 'p' | 'label';
And it could be used directly. Especially considering, that key and value are the same
There was a problem hiding this comment.
+ you could get rid of the extra tagMap with this approach
There was a problem hiding this comment.
String literal can be also separated (i.e AvailableTags = 'div' | ...;)
There was a problem hiding this comment.
@krau5, that's a great suggestion. Since the keys and values in tagMap are the same, we can simplify the code by using a union of string literals for the as prop and remove the need for tagMap. Here's how it could be refactored:
type AvailableTags = 'div' | 'span' | 'p' | 'label';
export type TextProps = {
children: React.ReactNode;
customRootClass?: string;
className?: string;
as?: AvailableTags;
} & React.ComponentProps<AvailableTags>;
const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return React.createElement(as, { className: `${rootClass} ${className}`, ...props }, children);
};This approach enhances type safety and simplifies the component by eliminating the unnecessary tagMap.
✏️ Learnings added
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:03.704Z
Learning: When the keys and values in a tag map are the same, prefer using a union of string literals for the `as` prop and eliminate the need for the tag map.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@krau5, that's a great point. Since the keys and values in the tagMap are the same, we can simplify the code by removing the tagMap and directly using the as prop with a union type. Here's how the code can be updated:
import React from 'react';
import { customClassSwitcher } from '~/core';
const COMPONENT_NAME = 'Text';
export type ValidTag = 'div' | 'span' | 'p' | 'label';
export type TextProps = {
children: React.ReactNode;
customRootClass?: string;
className?: string;
as?: ValidTag;
} & React.ComponentProps<ValidTag>;
const Text = ({
children,
customRootClass = '',
className = '',
as = 'p',
...props
}: TextProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
const Tag = as;
return React.createElement(
Tag,
{ className: `${rootClass} ${className}`, ...props },
children
);
};
Text.displayName = COMPONENT_NAME;
export default Text;This approach enhances type safety by limiting the as prop to specific tags and removes the unnecessary tagMap. Thanks for the suggestion!
✏️ Learnings added
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.705Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@krau5, that's an excellent suggestion! Since the keys and values are the same, we can simplify the code by using a union type for the as prop and eliminating the tagMap. Here's how we can modify the code:
type AvailableTags = 'div' | 'span' | 'p' | 'label';
export type TextProps = {
children: React.ReactNode;
customRootClass?: string;
className?: string;
as?: AvailableTags;
} & React.ComponentProps<AvailableTags>;
const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
return React.createElement(as, { className: `${rootClass} ${className}`, ...props }, children);
};
Text.displayName = COMPONENT_NAME;
export default Text;This approach enhances type safety and simplifies the component by removing the unnecessary tagMap. Thank you for your input!
✏️ Learnings added
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.437Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/components/ui/Text/Text.tsx
Outdated
| if (!componentList.find((item) => item.tag === as)) as='p' | ||
|
|
||
| const { tag: Tag } = componentList.find((item) => item.tag === as); | ||
| return <Tag className={`${rootClass} ${className}`} {...props}>{children}</Tag>; |
There was a problem hiding this comment.
Optimize tag selection logic
The current implementation searches the array twice and could throw an error if the tag isn't found:
- if (!componentList.find((item) => item.tag === as)) as='p'
- const { tag: Tag } = componentList.find((item) => item.tag === as);
- return <Tag className={`${rootClass} ${className}`} {...props}>{children}</Tag>;
+ const Tag = VALID_TAGS.includes(as) ? as : 'p';
+ return <Tag className={`${rootClass} ${className}`} {...props}>{children}</Tag>;This change:
- Eliminates double array search
- Simplifies the logic
- Prevents potential runtime errors
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 38-38: Operator '=' must be spaced.
(space-infix-ops)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/ui/Text/Text.tsx (2)
25-33: Fix formatting issues and simplify tag selectionThere are some formatting issues and the tag selection logic could be simplified.
Apply these changes:
-const Text = ({ children, customRootClass = '', className = '', as='p' , ...props }: TextProps) => { +const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); - - - const Tag = tagMap[as] || 'p'; - - return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children) - - + const Tag = tagMap[as] || 'p'; + return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children); };🧰 Tools
🪛 eslint
[error] 25-25: Operator '=' must be spaced.
(space-infix-ops)
[error] 28-29: More than 1 blank line not allowed.
(no-multiple-empty-lines)
Line range hint
5-7: Remove completed TODO commentsThe TODO comments about adding
asprop support have been addressed.Remove these resolved TODO comments:
-// TODO: Add as prop support -// TODO: Add a core reusable function to check and render an as prop🧰 Tools
🪛 eslint
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 25-25: Operator '=' must be spaced.
(space-infix-ops)
[error] 28-29: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 31-34: Block must not be padded by blank lines.
(padded-blocks)
[error] 33-34: More than 1 blank line not allowed.
(no-multiple-empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Text/Text.tsx(1 hunks)src/components/ui/Text/stories/Text.stories.js(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Text/Text.tsx
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 25-25: Operator '=' must be spaced.
(space-infix-ops)
[error] 28-29: More than 1 blank line not allowed.
(no-multiple-empty-lines)
🔇 Additional comments (2)
src/components/ui/Text/Text.tsx (2)
22-22: Enhance type safety for the as prop
Using the previously defined ValidTag type, update the TextProps:
- as?: string;
+ as?: ValidTag;11-16: 🛠️ Refactor suggestion
Fix formatting issues and enhance type safety
The tagMap implementation has some formatting issues and could benefit from improved type safety.
Apply these changes:
-const tagMap : { [key: string]: string } = {
+const VALID_TAGS = ['div', 'span', 'p', 'label'] as const;
+type ValidTag = typeof VALID_TAGS[number];
+
+const tagMap: Record<ValidTag, ValidTag> = {
div: 'div',
span: 'span',
p: 'p',
- label: 'label',
+ label: 'label'
};Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
| export const FaultyAs = { | ||
| args: { | ||
| className: 'text-gray-950', | ||
| as: "form" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the FaultyAs story approach
Instead of demonstrating invalid input with a story, consider adding proper TypeScript validation and error handling in the component itself. This story might give the impression that passing invalid tags is an acceptable use case.
Consider removing this story and adding stories for other valid tags instead:
-export const FaultyAs = {
- args: {
- className: 'text-gray-950',
- as: "form"
- }
-}
+export const AsSpan = {
+ args: {
+ className: 'text-gray-950',
+ as: "span"
+ }
+}
+export const AsLabel = {
+ args: {
+ className: 'text-gray-950',
+ as: "label"
+ }
+}📝 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.
| export const FaultyAs = { | |
| args: { | |
| className: 'text-gray-950', | |
| as: "form" | |
| } | |
| } | |
| export const AsSpan = { | |
| args: { | |
| className: 'text-gray-950', | |
| as: "span" | |
| } | |
| } | |
| export const AsLabel = { | |
| args: { | |
| className: 'text-gray-950', | |
| as: "label" | |
| } | |
| } |
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| const COMPONENT_NAME = 'Text'; | ||
|
|
||
| const tagMap : { [key: string]: string } = { |
There was a problem hiding this comment.
There is an utility type for such expressions
const tagMap: Record<string, string> = {}
| children: React.ReactNode; | ||
| customRootClass?: string; | ||
| className?: string; | ||
| as?: string; |
There was a problem hiding this comment.
You could just limit the type to
as?: 'div' | 'span' | 'p' | 'label';
And it could be used directly. Especially considering, that key and value are the same
| children: React.ReactNode; | ||
| customRootClass?: string; | ||
| className?: string; | ||
| as?: string; |
There was a problem hiding this comment.
+ you could get rid of the extra tagMap with this approach
| children: React.ReactNode; | ||
| customRootClass?: string; | ||
| className?: string; | ||
| as?: string; |
There was a problem hiding this comment.
String literal can be also separated (i.e AvailableTags = 'div' | ...;)
src/components/ui/Text/Text.tsx
Outdated
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
|
|
||
|
|
||
| const Tag = tagMap[as] || 'p'; |
There was a problem hiding this comment.
Considering my previous comments, you could just use as directly, so this one can be removed
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| const Tag = tagMap[as] || 'p'; | ||
|
|
||
| return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children) |
There was a problem hiding this comment.
You can use as directly. The default value for as should not be removed. Otherwise, you could put it like
return React.createElement(as ?? defaultTag, ...)
There was a problem hiding this comment.
Using as directly doesn't work with faulty or required tags
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/ui/Text/tests/Text.test.tsx (3)
36-54: Optimize test cases using test.eachThe current tests are repetitive with identical descriptions. Consider using
test.eachto make the tests more maintainable.-test('renders Text component with custom as prop', () => { - render(<Text as="span">I am Text!</Text>); - expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'SPAN'); -}); - -test('renders Text component with custom as prop', () => { - render(<Text as="div">I am Text!</Text>); - expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'DIV'); -}); -// ... more similar tests +test.each([ + ['span', 'SPAN'], + ['div', 'DIV'], + ['label', 'LABEL'], + ['p', 'P'], +])('renders Text component as %s tag', (as, expectedTag) => { + render(<Text as={as}>I am Text!</Text>); + expect(screen.getByText('I am Text!')).toHaveProperty('tagName', expectedTag); +});
65-65: Add missing semicolonAdd a semicolon at the end of the expect statement for consistency.
-expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'DIV') +expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'DIV');
74-74: Fix spacing around operators and bracesFix the inconsistent spacing in the test case.
-render(<Text as= {undefined} >I am Text!</Text>); +render(<Text as={undefined}>I am Text!</Text>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Text/Text.tsx(1 hunks)src/components/ui/Text/tests/Text.test.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/components/ui/Text/tests/Text.test.tsx (2)
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.599Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
src/components/ui/Text/Text.tsx (3)
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.599Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:03.895Z
Learning: When the keys and values in a tag map are the same, prefer using a union of string literals for the `as` prop and eliminate the need for the tag map.
🪛 Biome (1.9.4)
src/components/ui/Text/Text.tsx
[error] 23-23: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🪛 eslint
src/components/ui/Text/Text.tsx
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
[error] 23-23: Unexpected whitespace before semicolon.
(semi-spacing)
🔇 Additional comments (2)
src/components/ui/Text/Text.tsx (2)
11-11: 🛠️ Refactor suggestion
Remove unnecessary tagMap array
Based on previous discussions, since we're using string literals directly in the type definition, this array is redundant and can be removed.
If you need to validate the tag, you can directly check against the union type:
type ValidTag = 'div' | 'span' | 'p' | 'label';
const isValidTag = (tag: string): tag is ValidTag =>
['div', 'span', 'p', 'label'].includes(tag as ValidTag);17-18: 🛠️ Refactor suggestion
Update ComponentProps to use dynamic tag type
The component props should be based on the actual tag being used, not hardcoded to 'p'.
-} & React.ComponentProps<'p'>;
+} & React.ComponentProps<TextProps['as']>;⛔ Skipped due to learnings
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
src/components/ui/Text/Text.tsx
Outdated
| const Text = ({ children, customRootClass = '', className = '', as='p' , ...props }: TextProps) => { | ||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
|
|
||
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
| let Tag = 'p' | ||
| tagMap.map((tag) => tag === as && (Tag = as)) ; | ||
|
|
||
| return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children) |
There was a problem hiding this comment.
Simplify tag validation and fix formatting issues
The current implementation has several issues:
- Using
mapfor validation is incorrect and inefficient - Assignment within map expression is an anti-pattern
- Inconsistent spacing around operators
-const Text = ({ children, customRootClass = '', className = '', as='p' , ...props }: TextProps) => {
+const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => {
const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);
- let Tag = 'p'
- tagMap.map((tag) => tag === as && (Tag = as)) ;
-
- return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children)
+ return React.createElement(
+ as,
+ { className: `${rootClass} ${className}`, ...props },
+ children
+ );
};📝 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 Text = ({ children, customRootClass = '', className = '', as='p' , ...props }: TextProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | |
| let Tag = 'p' | |
| tagMap.map((tag) => tag === as && (Tag = as)) ; | |
| return React.createElement(Tag, { className: `${rootClass} ${className}`, ...props }, children) | |
| const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| return React.createElement( | |
| as, | |
| { className: `${rootClass} ${className}`, ...props }, | |
| children | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🪛 eslint
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
[error] 23-23: Unexpected whitespace before semicolon.
(semi-spacing)
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
| let Tag = 'p' | ||
| tagMap.map((tag) => tag === as && (Tag = as)) ; |
There was a problem hiding this comment.
This isn't being used anywhere
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'DIV') | ||
| }); | ||
|
|
||
| test('renders Text component with no as prop', () => { |
There was a problem hiding this comment.
Make the tests description more specific, they sound very generic
There was a problem hiding this comment.
will change the description
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| const COMPONENT_NAME = 'Text'; | ||
|
|
||
| const tagMap = ['div' , 'span' ,'p','label'] |
There was a problem hiding this comment.
It's not a map anymore, so we can probably just call it TAGS since they are sort of enums
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
| let Tag = 'p' | ||
| tagMap.map((tag) => tag === as && (Tag = as)) ; |
There was a problem hiding this comment.
If as doesn't exist inside TAGS, reinitialize as to p
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/Text/Text.tsx (1)
20-29: Simplify component implementationThe current implementation can be improved for clarity and consistency:
- Avoid renaming the
asprop toComponentas it might be misleading- Simplify the return statement
-const Text = ({ children, customRootClass = '', className = '', as: Component = 'p', ...props }: TextProps) => { +const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => { const rootClassName = customClassSwitcher(customRootClass, COMPONENT_NAME); - const component = TAGS.includes(Component) ? Component : 'p'; + const tag = VALID_TAGS.includes(as) ? as : 'p'; - return React.createElement( - component, - { className: `${rootClassName} ${className}`, ...props }, - children - ); + return React.createElement(tag, { className: `${rootClassName} ${className}`, ...props }, children); };src/components/ui/Text/tests/Text.test.js (1)
68-101: Fix formatting and consider additional edge casesThe edge case tests are good, but there are some formatting issues and potential additional cases to consider.
- Fix formatting in the array test case:
- render(<Text as = {["testing", "text" ]} >I am Text!</Text>); + render(<Text as={["testing", "text"]}>I am Text!</Text>);
- Consider adding these test cases:
- Test with empty string as prop
- Test with null as prop
- Test with object as prop
🧰 Tools
🪛 eslint
[error] 89-89: There should be no space before ']'.
(array-bracket-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Text/Text.tsx(1 hunks)src/components/ui/Text/tests/Text.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/ui/Text/Text.tsx (3)
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.599Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:03.895Z
Learning: When the keys and values in a tag map are the same, prefer using a union of string literals for the `as` prop and eliminate the need for the tag map.
🪛 eslint
src/components/ui/Text/tests/Text.test.js
[error] 89-89: There should be no space before ']'.
(array-bracket-spacing)
[error] 101-103: Block must not be padded by blank lines.
(padded-blocks)
🔇 Additional comments (4)
src/components/ui/Text/Text.tsx (2)
11-11: 🛠️ Refactor suggestion
Fix formatting and consider using string literals
The array declaration has inconsistent spacing. Additionally, based on previous feedback, consider using string literals directly in the type definition.
-const TAGS = ['div' , 'span' ,'p','label']
+type ValidTag = 'div' | 'span' | 'p' | 'label';Likely invalid or redundant comment.
17-17: 🛠️ Refactor suggestion
Improve type safety for the as prop
The current type allows any string, which is too permissive. Constrain it to only valid tags.
- as?: string;
+ as?: ValidTag;
} & React.ComponentProps<'p'>;⛔ Skipped due to learnings
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
src/components/ui/Text/tests/Text.test.js (2)
1-34: LGTM! Well-structured basic test cases
The basic test cases are comprehensive and well-organized, covering essential functionality like className, style, and custom attributes.
36-66: LGTM! Comprehensive testing of the as prop
The test cases thoroughly verify the rendering of different HTML tags and their combination with other props.
|
Refactored the code for better readability. Tests migrated to js addressing issue #611 changed |
src/components/ui/Text/Text.tsx
Outdated
| const Text = ({ children, customRootClass = '', className = '', as: Component = 'p', ...props }: TextProps) => { | ||
| const rootClassName = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
|
|
||
| const component = TAGS.includes(Component) ? Component : 'p'; |
There was a problem hiding this comment.
the variables here are getting confusing, we dont need Component, or component - both of these are just tags and not components
check if as is included in TAGS, else reassign default, simple.
src/components/ui/Text/Text.tsx
Outdated
|
|
||
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
| return React.createElement( | ||
| component, |
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); | ||
| }); | ||
|
|
||
| test('renders Text component with span as prop and custom data attribute', () => { |
There was a problem hiding this comment.
we can remove this, its already covered
| expect(screen.getByText('I am Text!')).toHaveAttribute('data-testid', 'text-data'); | ||
| }); | ||
|
|
||
| test('renders Text component with custom style and div as prop', () => { |
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'DIV') | ||
| }); | ||
|
|
||
| test('renders Text component with no as prop', () => { |
There was a problem hiding this comment.
"renders Text component with no props"
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); | ||
| }); | ||
|
|
||
| test('renders Text component with undefined as prop', () => { |
There was a problem hiding this comment.
renders Text component as a default <p> tag when "as" is undefined
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); | ||
| }); | ||
|
|
||
| test('renders Text component with random text as prop', () => { |
There was a problem hiding this comment.
renders Text component with default <p> tag when "as" prop is unsupported
| expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); | ||
| }); | ||
|
|
||
| test('renders Text component with number as prop', () => { |
There was a problem hiding this comment.
follow the same description as mentioned above for remaining cases below
|
suggested changes have been made 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/components/ui/Text/Text.tsx (1)
20-20: Fix operator spacingAdd spaces around the assignment operator.
-const Text = ({ children, customRootClass = '', className = '', as= 'p', ...props }: TextProps) => { +const Text = ({ children, customRootClass = '', className = '', as = 'p', ...props }: TextProps) => {🧰 Tools
🪛 eslint
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Text/tests/Text.test.js (3)
11-19: Consolidate redundant test casesThe first two test cases are testing similar behavior. Consider combining them into a single, more comprehensive test.
- test('renders Text component', () => { - render(<Text className='font-bold'>I am Text!</Text>); - expect(screen.getByText('I am Text!')).toBeInTheDocument(); - }); - - test('renders Text component with className', () => { - render(<Text className='font-bold'>I am Text!</Text>); - expect(screen.getByText('I am Text!')).toHaveClass('font-bold'); - }); + test('renders Text component with className correctly', () => { + const text = 'I am Text!'; + render(<Text className='font-bold'>{text}</Text>); + const element = screen.getByText(text); + expect(element).toBeInTheDocument(); + expect(element).toHaveClass('font-bold'); + });
56-57: Fix formatting inconsistenciesThere are a few formatting issues that should be addressed:
- Remove extra blank line at line 56
- Fix array spacing at line 78
- render(<Text as = {["testing", "text" ]} >I am Text!</Text>); + render(<Text as={["testing", "text"]}>I am Text!</Text>);Also applies to: 78-78
🧰 Tools
🪛 eslint
[error] 56-57: More than 1 blank line not allowed.
(no-multiple-empty-lines)
62-90: Improve organization of edge case testsConsider using a test.each pattern for the invalid
asprop tests to reduce repetition and improve maintainability.- test('renders Text component as a default <p> tag when "as" is undefined', () => { - render(<Text as={undefined} >I am Text!</Text>); - expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); - }); - // ... other invalid cases ... + test.each([ + ['undefined', undefined], + ['random text', 'text'], + ['number', 0], + ['array', ['testing', 'text']], + ['false', false], + ['true', true] + ])('renders as default <p> tag when "as" is %s', (_, invalidValue) => { + render(<Text as={invalidValue}>I am Text!</Text>); + expect(screen.getByText('I am Text!')).toHaveProperty('tagName', 'P'); + });🧰 Tools
🪛 eslint
[error] 78-78: There should be no space before ']'.
(array-bracket-spacing)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Text/Text.tsx(1 hunks)src/components/ui/Text/tests/Text.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/ui/Text/Text.tsx (3)
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.599Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:03.895Z
Learning: When the keys and values in a tag map are the same, prefer using a union of string literals for the `as` prop and eliminate the need for the tag map.
🪛 eslint
src/components/ui/Text/Text.tsx
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Text/tests/Text.test.js
[error] 56-57: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 78-78: There should be no space before ']'.
(array-bracket-spacing)
[error] 90-92: Block must not be padded by blank lines.
(padded-blocks)
🔇 Additional comments (1)
src/components/ui/Text/Text.tsx (1)
17-17: 🛠️ Refactor suggestion
Improve type safety of the as prop
Following the previous discussions, we should use a union type for better type safety.
- as?: string;
+ as?: 'div' | 'span' | 'p' | 'label';⛔ Skipped due to learnings
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
|
|
||
| const COMPONENT_NAME = 'Text'; | ||
|
|
||
| const TAGS = ['div' , 'span' ,'p','label'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix formatting and enhance type safety
The TAGS array has inconsistent spacing and could benefit from type safety improvements.
-const TAGS = ['div' , 'span' ,'p','label']
+const TAGS = ['div', 'span', 'p', 'label'] as const;📝 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 TAGS = ['div' , 'span' ,'p','label'] | |
| const TAGS = ['div', 'span', 'p', 'label'] as const; |
| if (!TAGS.includes(as)) as = 'p' | ||
|
|
||
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | ||
| return React.createElement( | ||
| as, | ||
| { className: `${rootClassName} ${className}`, ...props }, | ||
| children | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve tag validation logic
Avoid mutating the parameter directly and make the code more functional.
- if (!TAGS.includes(as)) as = 'p'
-
- return React.createElement(
- as,
- { className: `${rootClassName} ${className}`, ...props },
- children
- );
+ const tag = TAGS.includes(as) ? as : 'p';
+
+ return React.createElement(
+ tag,
+ { className: `${rootClassName} ${className}`, ...props },
+ children
+ );📝 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.
| if (!TAGS.includes(as)) as = 'p' | |
| return <p className={`${rootClass} ${className}`} {...props}>{children}</p>; | |
| return React.createElement( | |
| as, | |
| { className: `${rootClassName} ${className}`, ...props }, | |
| children | |
| ); | |
| const tag = TAGS.includes(as) ? as : 'p'; | |
| return React.createElement( | |
| tag, | |
| { className: `${rootClassName} ${className}`, ...props }, | |
| children | |
| ); |
| const Text = ({ children, customRootClass = '', className = '', as= 'p', ...props }: TextProps) => { | ||
| const rootClassName = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
|
|
||
| if (!TAGS.includes(as)) as = 'p' |
There was a problem hiding this comment.
as = TAGS.includes(as) ? as : 'p';
3 participants
Text has now
asprop which has default p tag and option of div, span, label and p tag. Wrote storybook example for with and withoutasprop. Using literal string forasprop as recommended by @krau5. Wrote test for text component covering theasprop. issue #605Summary by CodeRabbit
Summary by CodeRabbit
New Features
Textcomponent to render different HTML tags based on a newasprop, increasing flexibility in usage.WithAsandFaultyAs, for better demonstration of theTextcomponent's capabilities.Textcomponent, validating the newasprop functionality and various rendering scenarios.Bug Fixes
Textcomponent in Storybook by using a spread operator for props.