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

feat(charts): add RTL legend support #9570

Merged
merged 6 commits into from
Sep 22, 2023
Merged

feat(charts): add RTL legend support #9570

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb601f1
feat(charts): add RTL legend support
kmcfaul Sep 6, 2023
fd66344
clone data/label components
kmcfaul Sep 7, 2023
794520b
remove legend positions, add beta
kmcfaul Sep 19, 2023
62c1ca0
remove last position update
kmcfaul Sep 19, 2023
a4e0be6
update logic to account for text length
kmcfaul Sep 21, 2023
df4ddfe
update clones
kmcfaul Sep 22, 2023
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
21 changes: 21 additions & 0 deletions packages/react-charts/src/components/Chart/Chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { getPatternDefs, mergePatternData, useDefaultPatternProps } from '../ChartUtils/chart-patterns';
import { getChartTheme } from '../ChartUtils/chart-theme-types';
import { useEffect } from 'react';
import { ChartLabel } from '../ChartLabel/ChartLabel';
import { ChartPoint } from '../ChartPoint/ChartPoint';

/**
* Chart is a wrapper component that reconciles the domain for all its children, controls the layout of the chart,
Expand Down Expand Up @@ -284,6 +286,10 @@ export interface ChartProps extends VictoryChartProps {
* cases, the legend may not be visible until enough padding is applied.
*/
legendPosition?: 'bottom' | 'bottom-left' | 'right';
/**
* @beta Text direction of the legend labels.
*/
legendDirection?: 'ltr' | 'rtl';
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than a prop, should this auto switch when browser is RTL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per conversation, leaving this PR as is and opened #9656 to investigate adding logic to smartly set the default.

/**
* The maxDomain prop defines a maximum domain value for a chart. This prop is useful in situations where the maximum
* domain of a chart is static, while the minimum value depends on data or other variable information. If the domain
Expand Down Expand Up @@ -471,6 +477,7 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
legendComponent = <ChartLegend />,
legendData,
legendPosition = ChartCommonStyles.legend.position,
legendDirection = 'ltr',
name,
padding,
patternScale,
Expand Down Expand Up @@ -527,6 +534,20 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
...(name && { name: `${name}-${(legendComponent as any).type.displayName}` }),
orientation: legendOrientation,
theme,
...(legendDirection === 'rtl' && {
dataComponent: legendComponent.props.dataComponent ? (
React.cloneElement(legendComponent.props.dataComponent, { transform: 'translate(40)' })
) : (
<ChartPoint transform="translate(40)" />
)
}),
...(legendDirection === 'rtl' && {
labelComponent: legendComponent.props.labelComponent ? (
React.cloneElement(legendComponent.props.labelComponent, { direction: 'rtl', dx: '10' })
) : (
<ChartLabel direction="rtl" dx="10" />
)
}),
Copy link
Member

Choose a reason for hiding this comment

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

The legendDirection props look fine. However, I don't believe we should hard code the translate value. We need to calculate the length of the label in order to position the legend properly.

Screenshot 2023-09-21 at 1 55 59 PM

Copy link
Contributor Author

@kmcfaul kmcfaul Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The value of the translate prop isn't dependent on the text length, it's moving the Point a set distance from where the right aligned text ends which is static. There is no in-built "rtl" position for the Point, and without the translate the icons would show inside the text because it's still positioned relative to the text as "ltr".

The text issue shown in the screenshot could be resolved by adjusting the padding, which the legendPosition prop description kind of mentions but I think it's not for the same reason. I'll dig into the position calc logic - my thinking is that this is where we need to take the text length into account for rtl. It might also be possible to set it on the fly with the translate too, will test both methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlabrecq Updated the logic to account for text length for RTL legends, for very long text lengths you'll still need to adjust padding but that is what the position prop refers to being required.

I tried both ways of adjusting the legend position and updating the translate ended up being simpler with less piping required for props. The extra 30 in the label offset is to shift the text further over to push the icon out to the right properly.

Copy link
Member

@dlabrecq dlabrecq Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For some reason, this was not working in the preview; however, it's working locally. Suspect I may have been viewing an old preview.

Copy link
Member

@dlabrecq dlabrecq Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One more question... Why does the clone get translate(40), but the non-clone gets translate(legendXOffset)?

Screenshot 2023-09-21 at 5 03 55 PM

Shouldn't they both use translate(legendXOffset)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh forgot to update those, will fix that. Perhaps updating the branch again will fix the preview too

...legendComponent.props
});

Expand Down
21 changes: 21 additions & 0 deletions packages/react-charts/src/components/ChartBullet/ChartBullet.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { getBulletDomain } from './utils/chart-bullet-domain';
import { getBulletThemeWithLegendColorScale } from './utils/chart-bullet-theme';
import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { useEffect } from 'react';
import { ChartPoint } from '../ChartPoint/ChartPoint';
import { ChartLabel } from '../ChartLabel/ChartLabel';

/**
* ChartBullet renders a dataset as a bullet chart.
Expand Down Expand Up @@ -252,6 +254,10 @@ export interface ChartBulletProps {
* cases, the legend may not be visible until enough padding is applied.
*/
legendPosition?: 'bottom' | 'bottom-left' | 'right';
/**
* @beta Text direction of the legend labels.
*/
legendDirection?: 'ltr' | 'rtl';
/**
* The maxDomain prop defines a maximum domain value for a chart. This prop is useful in situations where the maximum
* domain of a chart is static, while the minimum value depends on data or other variable information. If the domain
Expand Down Expand Up @@ -509,6 +515,7 @@ export const ChartBullet: React.FunctionComponent<ChartBulletProps> = ({
legendComponent = <ChartLegend />,
legendItemsPerRow,
legendPosition = 'bottom',
legendDirection = 'ltr',
maxDomain,
minDomain,
name,
Expand Down Expand Up @@ -670,6 +677,20 @@ export const ChartBullet: React.FunctionComponent<ChartBulletProps> = ({
orientation: legendOrientation,
position: legendPosition,
theme,
...(legendDirection === 'rtl' && {
dataComponent: legendComponent.props.dataComponent ? (
React.cloneElement(legendComponent.props.dataComponent, { transform: 'translate(40)' })
) : (
<ChartPoint transform="translate(40)" />
)
}),
...(legendDirection === 'rtl' && {
labelComponent: legendComponent.props.labelComponent ? (
React.cloneElement(legendComponent.props.labelComponent, { direction: 'rtl', dx: '10' })
) : (
<ChartLabel direction="rtl" dx="10" />
)
}),
...legendComponent.props
});

Expand Down
6 changes: 6 additions & 0 deletions packages/react-charts/src/components/ChartDonut/ChartDonut.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,10 @@ export interface ChartDonutProps extends ChartPieProps {
* cases, the legend may not be visible until enough padding is applied.
*/
legendPosition?: 'bottom' | 'right';
/**
* @beta Text direction of the legend labels.
*/
legendDirection?: 'ltr' | 'rtl';
/**
* The name prop is typically used to reference a component instance when defining shared events. However, this
* optional prop may also be applied to child elements as an ID prefix. This is a workaround to ensure Victory
Expand Down Expand Up @@ -573,6 +577,7 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
containerComponent = <ChartContainer />,
innerRadius,
legendPosition = ChartCommonStyles.legend.position,
legendDirection = 'ltr',
name,
padAngle,
padding,
Expand Down Expand Up @@ -698,6 +703,7 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
innerRadius={chartInnerRadius > 0 ? chartInnerRadius : 0}
key="pf-chart-donut-pie"
legendPosition={legendPosition}
legendDirection={legendDirection}
name={name}
padAngle={padAngle !== undefined ? padAngle : getPadAngle}
padding={padding}
Expand Down
6 changes: 6 additions & 0 deletions packages/react-charts/src/components/ChartDonutUtilization/ChartDonutUtilization.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,10 @@ export interface ChartDonutUtilizationProps extends ChartDonutProps {
* cases, the legend may not be visible until enough padding is applied.
*/
legendPosition?: 'bottom' | 'right';
/**
* @beta Text direction of the legend labels.
*/
legendDirection?: 'ltr' | 'rtl';
/**
* The labelRadius prop defines the radius of the arc that will be used for positioning each slice label.
* If this prop is not set, the label radius will default to the radius of the pie + label padding.
Expand Down Expand Up @@ -589,6 +593,7 @@ export const ChartDonutUtilization: React.FunctionComponent<ChartDonutUtilizatio
invert = false,
isStatic = true,
legendPosition = ChartCommonStyles.legend.position,
legendDirection = 'ltr',
padding,
standalone = true,
themeColor,
Expand Down Expand Up @@ -679,6 +684,7 @@ export const ChartDonutUtilization: React.FunctionComponent<ChartDonutUtilizatio
height={height}
key="pf-chart-donut-utilization"
legendPosition={legendPosition}
legendDirection={legendDirection}
padding={padding}
standalone={false}
theme={getThresholdTheme()}
Expand Down
23 changes: 21 additions & 2 deletions packages/react-charts/src/components/ChartPie/ChartPie.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { getPatternDefs, useDefaultPatternProps } from '../ChartUtils/chart-patterns';
import { getTheme } from '../ChartUtils/chart-theme';
import { useEffect } from 'react';
import { ChartPoint } from '../ChartPoint/ChartPoint';
import { ChartLabel } from '../ChartLabel/ChartLabel';

/**
* ChartPie renders a dataset as a pie chart.
Expand Down Expand Up @@ -333,6 +335,10 @@ export interface ChartPieProps extends VictoryPieProps {
* cases, the legend may not be visible until enough padding is applied.
*/
legendPosition?: 'bottom' | 'right';
/**
* @beta Text direction of the legend labels.
*/
legendDirection?: 'ltr' | 'rtl';
/**
* The name prop is typically used to reference a component instance when defining shared events. However, this
* optional prop may also be applied to child elements as an ID prefix. This is a workaround to ensure Victory
Expand Down Expand Up @@ -498,16 +504,15 @@ export const ChartPie: React.FunctionComponent<ChartPieProps> = ({
legendComponent = <ChartLegend />,
legendData,
legendPosition = ChartCommonStyles.legend.position,
legendDirection = 'ltr',
name,
patternScale,
patternUnshiftIndex,

padding,
radius,
standalone = true,
style,
themeColor,

// destructure last
theme = getTheme(themeColor),
labelComponent = allowTooltip ? (
Expand Down Expand Up @@ -583,6 +588,20 @@ export const ChartPie: React.FunctionComponent<ChartPieProps> = ({
key: 'pf-chart-pie-legend',
orientation: legendOrientation,
theme,
...(legendDirection === 'rtl' && {
dataComponent: legendComponent.props.dataComponent ? (
React.cloneElement(legendComponent.props.dataComponent, { transform: 'translate(80)' })
) : (
<ChartPoint transform="translate(80)" />
)
}),
...(legendDirection === 'rtl' && {
labelComponent: legendComponent.props.labelComponent ? (
React.cloneElement(legendComponent.props.labelComponent, { direction: 'rtl', dx: '50' })
) : (
<ChartLabel direction="rtl" dx="50" />
)
}),
...legendComponent.props
});

Expand Down