Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ Refacto price #236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

♻️ Refacto price #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 28 additions & 36 deletions packages/react/components/price/Price.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import clsx from 'clsx'
import { PriceProps } from './PriceProps'
import { has, is } from '@/services/classify'
import { Text, TextMarkup } from '../text'
import { Alignable, TypographyBold, TypographyColor } from '@/objects'
import { Alignable } from '@/objects'
import { checkCents } from './PriceHelpers'
import { hashClass } from '@/helpers'
import { useTrilogyContext } from '@/context'
import { PriceLevel } from './PriceEnum'
import { useMemo } from 'react'

/**
* Price Component
Expand All @@ -18,7 +20,6 @@ import { useTrilogyContext } from '@/context'
* @param inverted {boolean} Inverted Price Color
* @param children {React.ReactNode}
* @param align {Alignable} Price alignement
* @param inline {boolean} Inline display Price
* @param accessibilityLabel {string} Accessibility label
* @param overline {string} Price overline
* @param oldAmount {boolean} old Amount Price
Expand All @@ -33,7 +34,7 @@ const Price = ({
mention,
period,
hideCents = false,
level,
level = PriceLevel.ONE,
inverted,
align,
accessibilityLabel,
Expand All @@ -52,52 +53,44 @@ const Price = ({

let amountComponent = null
let oldAmountComponent = null
const tagAmountComponent = null

const getAmountContent = useMemo(
() => (amount: number, cents: string) =>
(
<>
<Text markup={TextMarkup.SPAN}>{amount}</Text>
<span className={hashClass(styled, clsx('price-details'))}>
<span className={hashClass(styled, clsx('cents'))}>
&nbsp;€{hideCents ? '' : cents}
{mention && <sup>{mention}</sup>}
</span>
{period && <span className={hashClass(styled, clsx('period'))}>/{period}</span>}
</span>
</>
),
[hideCents, mention, period, styled],
)

if (oldAmount) {
const isNegativeStrike = oldAmount && oldAmount < 0
const absoluteAmountStrike = oldAmount && Math.abs(oldAmount)
const absoluteWholeStrike = absoluteAmountStrike && Math.floor(absoluteAmountStrike)
const wholeStrike = isNegativeStrike && absoluteWholeStrike ? -absoluteWholeStrike : absoluteWholeStrike
const wholeStrike: number = oldAmount < 0 ? Math.ceil(oldAmount) : Math.floor(oldAmount)

let cents = checkCents(absoluteAmountStrike.toString().split(/[.,]/)[1]?.substring(0, 2) || '')
cents = (cents && cents.length === 1 && `${cents}0`) || cents
const centsDisplayed = (!hideCents && `€${cents}`) || '€'
const cents: string = checkCents(oldAmount.toString().split('.')[1]?.substring(0, 2) ?? '00')

oldAmountComponent = (
<span aria-hidden='true' className={classesStrike} {...others}>
<Text markup={TextMarkup.SPAN}>{`${wholeStrike}`}</Text>
<span className={hashClass(styled, clsx('price-details'))}>
<span className={hashClass(styled, clsx('cents'))}>
{centsDisplayed === '€' ? <>&nbsp;{centsDisplayed}</> : centsDisplayed}
{mention && <sup>{mention}</sup>}
</span>
{period && <span className={hashClass(styled, clsx('period'))}>/{period}</span>}
</span>
{getAmountContent(wholeStrike, cents)}
</span>
)
}

if (amount) {
const isNegative = amount < 0
const absoluteAmount = Math.abs(amount)
const absoluteWhole = Math.floor(absoluteAmount)
const whole = isNegative ? -absoluteWhole : absoluteWhole
const whole: number = amount < 0 ? Math.ceil(amount) : Math.floor(amount)

let cents = checkCents(absoluteAmount.toString().split(/[.,]/)[1]?.substring(0, 2) || '')
cents = (cents && cents.length === 1 && `${cents}0`) || cents
const centsDisplayed = (!hideCents && `€${cents}`) || '€'
const cents: string = checkCents(amount.toString().split('.')[1]?.substring(0, 2) ?? '00')

amountComponent = (
<span aria-hidden='true' aria-label={accessibilityLabel} className={classes} {...others}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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-label attribute 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

<Text markup={TextMarkup.SPAN}>{`${whole}`}</Text>
<span className={hashClass(styled, clsx('price-details'))}>
<span className={hashClass(styled, clsx('cents'))}>
{centsDisplayed === '€' ? <>&nbsp;{centsDisplayed}</> : centsDisplayed}
{mention && <sup>{mention}</sup>}
</span>
{period && <span className={hashClass(styled, clsx('period'))}>/{period}</span>}
</span>
{getAmountContent(whole, cents)}
</span>
)
}
Expand All @@ -109,7 +102,7 @@ const Price = ({
styled,
clsx(
'price-container',
is(`level-${level || '1'}`),
is(`level-${level}`),
(align == Alignable.ALIGNED_START && is('justified-left')) ||
(align == Alignable.ALIGNED_CENTER && is('justified-center')) ||
(align == Alignable.ALIGNED_END && is('justified-right')) ||
Expand All @@ -120,7 +113,6 @@ const Price = ({
{overline && <p className={hashClass(styled, clsx('overline'))}>{overline}</p>}
{oldAmountComponent}
{amountComponent}
{tagAmountComponent}
{accessibilityLabel && <p className='sr-only'>{accessibilityLabel}</p>}
</div>
)
Expand Down
13 changes: 9 additions & 4 deletions packages/react/components/price/PriceHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/**
* 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 => {
let result = text
if (text.length === 1) {
result += '0'
if (!/^\d+$/.test(text)) {
throw new Error('Input must contain only digits')
}
return result
return text.length === 1 ? `${text}0` : text
}