♻️ Refacto price #236
♻️ Refacto price #236
Conversation
WalkthroughThe pull request involves refactoring the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 4
🧹 Nitpick comments (3)
packages/react/components/price/PriceHelpers.ts (1)
1-2: LGTM! Clean and concise implementation.The simplified ternary operation makes the code more readable while maintaining the same functionality.
Consider adding:
- Input validation to ensure
textcontains only digits- JSDoc documentation explaining the function's purpose
+/** + * Ensures cents are displayed with 2 digits by padding with a trailing zero if needed. + * @param text - The cents portion of a price (e.g., "5" or "50") + * @returns The padded cents string (e.g., "50" or "50") + * @throws {Error} If text contains non-digit characters + */ export const checkCents = (text: string): string => { + if (!/^\d+$/.test(text)) { + throw new Error('Input must contain only digits'); + } return text.length === 1 ? `${text}0` : text }packages/react/components/price/Price.tsx (2)
56-69: Fix typo and consider performance optimization.
- The function name has a typo:
getAmoutContentshould begetAmountContent- Consider memoizing this function since it creates JSX elements that could be reused across renders.
- const getAmoutContent = (amount: number, cents: string) => { + const getAmountContent = React.useMemo( + () => (amount: number, cents: string) => { return ( <> <Text markup={TextMarkup.SPAN}>{amount}</Text> <span className={hashClass(styled, clsx('price-details'))}> <span className={hashClass(styled, clsx('cents'))}> €{hideCents ? '' : cents} {mention && <sup>{mention}</sup>} </span> {period && <span className={hashClass(styled, clsx('period'))}>/{period}</span>} </span> </> ) + }, + [hideCents, mention, period, styled] + )
Line range hint
102-106: Simplify alignment class logic.The alignment class logic could be simplified using an object mapping or switch statement for better readability.
+ const alignmentClasses = { + [Alignable.ALIGNED_START]: 'justified-left', + [Alignable.ALIGNED_CENTER]: 'justified-center', + [Alignable.ALIGNED_END]: 'justified-right', + } <div id={id} className={hashClass( styled, clsx( 'price-container', is(`level-${level}`), - (align == Alignable.ALIGNED_START && is('justified-left')) || - (align == Alignable.ALIGNED_CENTER && is('justified-center')) || - (align == Alignable.ALIGNED_END && is('justified-right')) || - '', + align && is(alignmentClasses[align] || ''), ), )} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/components/price/Price.tsx(4 hunks)packages/react/components/price/PriceHelpers.ts(1 hunks)
🔇 Additional comments (1)
packages/react/components/price/Price.tsx (1)
36-36: LGTM! Good default prop value addition.
Adding a default value for the level prop is a good practice for backward compatibility and makes the prop optional.
| let cents = checkCents(absoluteAmount.toString().split(/[.,]/)[1]?.substring(0, 2) || '') | ||
| cents = (cents && cents.length === 1 && `${cents}0`) || cents | ||
| const centsDisplayed = (!hideCents && `€${cents}`) || '€' | ||
| const cents = checkCents(amount.toString().split('.')[1]?.substring(0, 2) ?? '00') | ||
|
|
||
| amountComponent = ( | ||
| <span aria-hidden='true' aria-label={accessibilityLabel} className={classes} {...others}> |
There was a problem hiding this comment.
💡 Codebase verification
Remove redundant aria-label attribute
The component has redundant accessibility attributes. The aria-hidden="true" span should not have an aria-label since it's explicitly hidden from screen readers. The accessibility is already properly handled by the sr-only paragraph element.
- Remove the
aria-labelattribute from line 89:
<span aria-hidden='true' className={classes} {...others}>🔗 Analysis chain
Verify aria-label implementation.
The component uses both aria-label and a separate element with sr-only class for the same content. This might be redundant.
Also applies to: 113-113
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other aria-label usage patterns in the codebase
rg -A 2 'aria-label.*sr-only' --type tsx
Length of output: 70
Script:
#!/bin/bash
# Check for aria-label and sr-only usage patterns in the codebase, adjusting file type
rg -t ts -t tsx 'aria-label.*sr-only|sr-only.*aria-label' -A 2
Length of output: 92
Script:
#!/bin/bash
# Get the full Price component implementation to understand the accessibility pattern
ast-grep --pattern 'export $_ function Price' -A 50
Length of output: 53
Script:
#!/bin/bash
# Look for sr-only class usage in the codebase
rg -t ts -t tsx 'sr-only' -A 2
Length of output: 58
Script:
#!/bin/bash
# Look for sr-only and aria-label patterns using just typescript extension
rg 'sr-only|aria-label' -t ts -A 2
Length of output: 6040
Script:
#!/bin/bash
# Get the Price component implementation using a more specific pattern
ast-grep --pattern 'function Price($$$) {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Get the content of the Price component file
cat packages/react/components/price/Price.tsx
Length of output: 3706
FelixLgr
force-pushed
the
clean/price
branch
from
4e64b6c to
5fd38f7
Compare
Summary by CodeRabbit
New Features
levelprop in thePricecomponent is now set toPriceLevel.ONE.Bug Fixes
Refactor
Pricecomponent for rendering amounts and cents.checkCentsfunction for more concise logic and added validation for numeric input.Documentation