feat(ColumnChart): add alignLabels property#8534
feat(ColumnChart): add alignLabels property#8534skythefire wants to merge 2 commits intoUI5:mainfrom
Conversation
|
|
There was a problem hiding this comment.
Hi @skythefire
thanks a lot for the contribution! Please see my review below.
BarCharthardcodes'insideRight'the same way. Both are independent wrappers around recharts'BarChart(no shared base). Please addalignLabelstoBarChartas well (default'insideRight').BulletChartandComposedChartalso use theChartDataLabelcomponent. Due to the type change a typescript error is now thrown. Please adjust the types forlabelPositionin these components as well. You can check if there are build errors locally by runningyarn build.
| /** | ||
| * Alignment of the labels of the data points. Can be one of the following: `"center"`, `"insideTop"`, `"insideTopRight"`, `"insideRight"`, `"insideBottomRight"`, `"insideBottom"`, `"insideBottomLeft"`, `"insideLeft"`, `"insideTopLeft"` | ||
| * | ||
| * @default 'center' |
There was a problem hiding this comment.
The hardcoded position was 'insideTop', but the new default is 'center'. This silently changes rendering for existing users. Please change the default to 'insideTop'.
| * @default 'center' | |
| * @default 'insideTop' |
| ChartPlaceholder, | ||
| syncId, | ||
| children, | ||
| alignLabels = 'center', |
There was a problem hiding this comment.
Default value change. (see comment above)
| alignLabels = 'center', | |
| alignLabels = 'insideTop', |
| * | ||
| * @default 'center' | ||
| */ | ||
| alignLabels: |
There was a problem hiding this comment.
Prop should be optional.
| alignLabels: | |
| alignLabels?: |
| | 'center' | ||
| | 'insideTop' | ||
| | 'insideTopRight' | ||
| | 'insideRight' | ||
| | 'insideBottomRight' | ||
| | 'insideBottom' | ||
| | 'insideBottomLeft' | ||
| | 'insideLeft' | ||
| | 'insideTopLeft'; |
There was a problem hiding this comment.
As this is only a pass through implementation of the recharts prop, you can reuse the type here.
import type { LabelProps } from 'recharts';
alignLabels?: LabelProps['position'];
| import { ThemingParameters } from '@ui5/webcomponents-react-base/ThemingParameters'; | ||
| import { createElement } from 'react'; | ||
| import { Label } from 'recharts'; | ||
| import type { LabelPosition } from 'recharts/types/component/Label.js'; |
There was a problem hiding this comment.
Avoid deep imports when possible, as they are mostly not considered stable. The import could break if the internal structure of the recharts library changes for example.
import type { LabelProps } from 'recharts';
position?: LabelProps['position'];
| value={formattedLabel} | ||
| /> | ||
| ); | ||
| return <Label viewBox={viewBox} {...props} fill={fill} stroke={'none'} content={undefined} value={formattedLabel} />; |
There was a problem hiding this comment.
This is a pre-existing issue, but since you're already in this file: the old {...(props as any)} hid the fact that config, chartType, isBigDataSet get spread onto <Label>. Now that as any is removed, could you also destructure those out?
const { config, chartType, isBigDataSet, ...labelProps } = props;
return <Label viewBox={viewBox} {...labelProps} fill={fill} stroke={'none'} content={undefined} value={formattedLabel} />;2 participants
Fixes: #3421
Thank you for your contribution! 👏
To get it merged faster, kindly review the checklist below:
Pull Request Checklist