Charts: Add TrendIndicator component #46213
Conversation
annacmc
added
[Type] Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Pull request overview
This PR adds a new TrendIndicator component to the Charts package for displaying directional trend information with visual indicators (arrows) and values. The component supports three trend directions (up, down, neutral) with corresponding color coding and optional icons.
Key Changes:
- New reusable
TrendIndicatorcomponent with TypeScript types and SCSS styling - Package.json exports configuration for tree-shaking support
- Comprehensive test coverage and Storybook stories for visual documentation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Exports TrendIndicator component and types from main package entry |
src/components/trend-indicator/types.ts |
Defines TypeScript types for TrendDirection and TrendIndicatorProps |
src/components/trend-indicator/trend-indicator.tsx |
Implements core component with Icon subcomponent, color mapping, and rendering logic |
src/components/trend-indicator/trend-indicator.module.scss |
Provides responsive styling with flexbox layout and BEM naming convention |
src/components/trend-indicator/test/trend-indicator.test.tsx |
Adds unit tests covering rendering, icons, and className prop handling |
src/components/trend-indicator/stories/index.stories.tsx |
Creates Storybook stories for Up, Down, and Neutral trend states |
src/components/trend-indicator/index.ts |
Re-exports component and types for clean import paths |
package.json |
Adds export paths and TypeScript type definitions for the new component |
changelog/add-charts-new-trend-indicator |
Documents the addition as a minor version change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import clsx from 'clsx'; | ||
| import styles from './trend-indicator.module.scss'; | ||
| import type { TrendIndicatorProps, TrendDirection } from './types'; | ||
|
|
||
| const COLORS: Record< TrendDirection, string > = { | ||
| up: '#1a8917', | ||
| down: '#d63638', | ||
| neutral: '#646970', | ||
| }; | ||
|
|
||
| const Icon = ( { direction }: { direction: TrendDirection } ) => { | ||
| if ( direction === 'neutral' ) { | ||
| return null; | ||
| } | ||
|
|
||
| const isUp = direction === 'up'; | ||
| return ( | ||
| <svg | ||
| className={ styles[ 'trend-indicator__icon' ] } | ||
| viewBox="0 0 16 16" | ||
| fill="none" | ||
| role="img" | ||
| aria-hidden="true" | ||
| > | ||
| <path | ||
| d={ isUp ? 'M8 13V3M4 7l4-4 4 4' : 'M8 3v10M4 9l4 4 4-4' } | ||
| stroke="currentColor" | ||
| strokeWidth="1.5" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| /> | ||
| </svg> | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * TrendIndicator displays a directional trend with a value. | ||
| * Used to show percentage changes or growth metrics. | ||
| * | ||
| * @param {TrendIndicatorProps} props - Component props | ||
| * @return {JSX.Element} The rendered trend indicator | ||
| */ | ||
| export function TrendIndicator( { direction, value, className, style }: TrendIndicatorProps ) { | ||
| return ( | ||
| <span | ||
| className={ clsx( styles[ 'trend-indicator' ], className ) } | ||
| style={ { ...style, color: COLORS[ direction ] } } | ||
| > | ||
| <Icon direction={ direction } /> | ||
| <span className={ styles[ 'trend-indicator__value' ] }>{ value }</span> | ||
| </span> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The component should use internationalization (i18n) for the accessible label text. Other components in this package use the @wordpress/i18n library's __() function for translatable strings (see bar-chart.tsx, line-chart.tsx).
Example implementation:
import { __ } from '@wordpress/i18n';
// In the component:
const getDirectionLabel = ( direction: TrendDirection ): string => {
switch ( direction ) {
case 'up':
return __( 'Increase', 'jetpack-charts' );
case 'down':
return __( 'Decrease', 'jetpack-charts' );
case 'neutral':
return __( 'No change', 'jetpack-charts' );
}
};This ensures the component works correctly for non-English users.
| className={ styles[ 'trend-indicator__icon' ] } | ||
| viewBox="0 0 16 16" | ||
| fill="none" | ||
| role="img" |
There was a problem hiding this comment.
The SVG icon has conflicting ARIA attributes. Setting both role="img" and aria-hidden="true" is contradictory - role="img" exposes the element to assistive technologies while aria-hidden="true" hides it.
Since the icon is decorative and the semantic meaning should be conveyed through the parent component's aria-label, the icon should only have aria-hidden="true":
<svg
className={ styles[ 'trend-indicator__icon' ] }
viewBox="0 0 16 16"
fill="none"
aria-hidden="true"
>Remove the role="img" attribute.
| role="img" |
| render( <TrendIndicator direction="up" value="+10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders icon for down direction', () => { | ||
| render( <TrendIndicator direction="down" value="-10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'does not render icon for neutral direction', () => { | ||
| render( <TrendIndicator direction="neutral" value="0%" /> ); | ||
|
|
||
| expect( screen.queryByRole( 'img', { hidden: true } ) ).not.toBeInTheDocument(); |
There was a problem hiding this comment.
This test is querying for role="img" which should be removed from the component (see the accessibility issue in the main component file). After removing role="img" from the SVG, these tests will need to be updated to use a different query method.
Consider using container.querySelector('svg') instead:
it( 'renders icon for up direction', () => {
const { container } = render( <TrendIndicator direction="up" value="+10%" /> );
const svg = container.querySelector( 'svg' );
expect( svg ).toBeInTheDocument();
expect( svg ).toHaveAttribute( 'aria-hidden', 'true' );
} );| render( <TrendIndicator direction="up" value="+10%" /> ); | |
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | |
| } ); | |
| it( 'renders icon for down direction', () => { | |
| render( <TrendIndicator direction="down" value="-10%" /> ); | |
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | |
| } ); | |
| it( 'does not render icon for neutral direction', () => { | |
| render( <TrendIndicator direction="neutral" value="0%" /> ); | |
| expect( screen.queryByRole( 'img', { hidden: true } ) ).not.toBeInTheDocument(); | |
| const { container } = render( <TrendIndicator direction="up" value="+10%" /> ); | |
| const svg = container.querySelector('svg'); | |
| expect( svg ).toBeInTheDocument(); | |
| expect( svg ).toHaveAttribute( 'aria-hidden', 'true' ); | |
| } ); | |
| it( 'renders icon for down direction', () => { | |
| const { container } = render( <TrendIndicator direction="down" value="-10%" /> ); | |
| const svg = container.querySelector('svg'); | |
| expect( svg ).toBeInTheDocument(); | |
| expect( svg ).toHaveAttribute( 'aria-hidden', 'true' ); | |
| } ); | |
| it( 'does not render icon for neutral direction', () => { | |
| const { container } = render( <TrendIndicator direction="neutral" value="0%" /> ); | |
| const svg = container.querySelector('svg'); | |
| expect( svg ).not.toBeInTheDocument(); |
| render( <TrendIndicator direction="up" value="+10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders icon for down direction', () => { | ||
| render( <TrendIndicator direction="down" value="-10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'does not render icon for neutral direction', () => { | ||
| render( <TrendIndicator direction="neutral" value="0%" /> ); | ||
|
|
||
| expect( screen.queryByRole( 'img', { hidden: true } ) ).not.toBeInTheDocument(); |
There was a problem hiding this comment.
This test is querying for role="img" which should be removed from the component (see the accessibility issue in the main component file). After removing role="img" from the SVG, these tests will need to be updated to use a different query method.
Consider using container.querySelector('svg') instead:
it( 'does not render icon for neutral direction', () => {
const { container } = render( <TrendIndicator direction="neutral" value="0%" /> );
expect( container.querySelector( 'svg' ) ).not.toBeInTheDocument();
} );| render( <TrendIndicator direction="up" value="+10%" /> ); | |
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | |
| } ); | |
| it( 'renders icon for down direction', () => { | |
| render( <TrendIndicator direction="down" value="-10%" /> ); | |
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | |
| } ); | |
| it( 'does not render icon for neutral direction', () => { | |
| render( <TrendIndicator direction="neutral" value="0%" /> ); | |
| expect( screen.queryByRole( 'img', { hidden: true } ) ).not.toBeInTheDocument(); | |
| const { container } = render( <TrendIndicator direction="up" value="+10%" /> ); | |
| expect( container.querySelector('svg') ).toBeInTheDocument(); | |
| } ); | |
| it( 'renders icon for down direction', () => { | |
| const { container } = render( <TrendIndicator direction="down" value="-10%" /> ); | |
| expect( container.querySelector('svg') ).toBeInTheDocument(); | |
| } ); | |
| it( 'does not render icon for neutral direction', () => { | |
| const { container } = render( <TrendIndicator direction="neutral" value="0%" /> ); | |
| expect( container.querySelector('svg') ).not.toBeInTheDocument(); |
| <span | ||
| className={ clsx( styles[ 'trend-indicator' ], className ) } | ||
| style={ { ...style, color: COLORS[ direction ] } } | ||
| > | ||
| <Icon direction={ direction } /> | ||
| <span className={ styles[ 'trend-indicator__value' ] }>{ value }</span> | ||
| </span> |
There was a problem hiding this comment.
The component lacks an aria-label or accessible text to convey the meaning of the trend to screen reader users. Users relying on assistive technologies would only hear the value (e.g., "+14%") without understanding whether it represents an increase, decrease, or neutral trend.
Consider adding an aria-label that describes the full meaning, such as:
<span
className={ clsx( styles[ 'trend-indicator' ], className ) }
style={ { ...style, color: COLORS[ direction ] } }
aria-label={ `${ direction === 'up' ? 'Increase' : direction === 'down' ? 'Decrease' : 'No change' }: ${ value }` }
>Alternatively, use a visually hidden text element to provide context alongside the visual indicator.
| const COLORS: Record< TrendDirection, string > = { | ||
| up: '#1a8917', | ||
| down: '#d63638', | ||
| neutral: '#646970', | ||
| }; |
There was a problem hiding this comment.
[nitpick] The hardcoded color values don't integrate with the existing chart theming system. The charts package uses a theme provider (GlobalChartsProvider) that allows consumers to customize colors through themes (see themes.ts).
For consistency with other components like ConversionFunnelChart (which uses positiveChangeColor and negativeChangeColor from theme settings), consider:
- Adding
trendIndicatorcolors to the theme configuration inthemes.ts:
trendIndicator: {
upColor: '#1a8917',
downColor: '#d63638',
neutralColor: '#646970',
}- Accessing theme colors via the context:
const { trendIndicatorSettings } = useChartTheme();
const COLORS: Record< TrendDirection, string > = {
up: trendIndicatorSettings?.upColor || '#1a8917',
down: trendIndicatorSettings?.downColor || '#d63638',
neutral: trendIndicatorSettings?.neutralColor || '#646970',
};This would allow consumers to customize trend indicator colors to match their design system while maintaining sensible defaults.
| import { render, screen } from '@testing-library/react'; | ||
| import { TrendIndicator } from '../trend-indicator'; | ||
|
|
||
| describe( 'TrendIndicator', () => { | ||
| it( 'renders up trend with value', () => { | ||
| render( <TrendIndicator direction="up" value="+14%" /> ); | ||
|
|
||
| expect( screen.getByText( '+14%' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders down trend with value', () => { | ||
| render( <TrendIndicator direction="down" value="-5%" /> ); | ||
|
|
||
| expect( screen.getByText( '-5%' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders neutral trend with value', () => { | ||
| render( <TrendIndicator direction="neutral" value="0%" /> ); | ||
|
|
||
| expect( screen.getByText( '0%' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders icon for up direction', () => { | ||
| render( <TrendIndicator direction="up" value="+10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'renders icon for down direction', () => { | ||
| render( <TrendIndicator direction="down" value="-10%" /> ); | ||
|
|
||
| expect( screen.getByRole( 'img', { hidden: true } ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'does not render icon for neutral direction', () => { | ||
| render( <TrendIndicator direction="neutral" value="0%" /> ); | ||
|
|
||
| expect( screen.queryByRole( 'img', { hidden: true } ) ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'applies custom className', () => { | ||
| const { container } = render( | ||
| <TrendIndicator direction="up" value="+10%" className="custom-class" /> | ||
| ); | ||
|
|
||
| // eslint-disable-next-line testing-library/no-node-access | ||
| expect( container.firstChild ).toHaveClass( 'custom-class' ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Tests are missing for the accessibility concern of providing semantic meaning to screen reader users. Consider adding a test to verify that the component provides appropriate accessible text:
it( 'provides accessible label for screen readers', () => {
const { container } = render(
<TrendIndicator direction="up" value="+14%" />
);
const indicator = container.firstChild as HTMLElement;
expect( indicator ).toHaveAttribute( 'aria-label' );
expect( indicator.getAttribute( 'aria-label' ) ).toMatch( /increase/i );
} );This would ensure the component remains accessible as it evolves.
Code Coverage Summary3 files are newly checked for coverage.
|
Proposed changes:
TrendIndicatorcomponent for displaying directional trends with valuesup(green),down(red),neutral(gray)Other information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
Up: Should display green arrow pointing up with "+14%"Down: Should display red arrow pointing down with "-5%"Neutral: Should display gray text "0%" with no iconclassNameandstyleprops for customizationpnpm test -- src/components/trend-indicatorpnpm buildand verify trend-indicator outputs are generated