-
Notifications
You must be signed in to change notification settings - Fork 12
feat: dual axis support in legend #122
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.
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
Conversation
src/core/chart-container.tsx
Outdated
| filter?: React.ReactNode; | ||
| navigator?: React.ReactNode; | ||
| legend?: React.ReactNode; | ||
| firstLegend?: React.ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather use primaryLegend and secondaryLegend instead of first/second here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in other places the second/secondary legend is referred to as "opposite" if I understand correctly, so let's try and unify the naming
| const [selectedIndex, setSelectedIndex] = useState<number>(0); | ||
| const [tooltipItemId, setTooltipItemId] = useState<string | null>(null); | ||
|
|
||
| const filteredItems = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we filter items here instead of provide the prepared set of items from the outside?
| const SCROLL_DELAY = 100; | ||
| const SHOULD_STACK_SIZE = 400; | ||
|
|
||
| export type ChartLegendType = "bottom" | "stacked" | "stacked-top" | "stacked-bottom" | "bottom-left" | "bottom-right"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels unnecessary - let's try to simplify it. I think the legend component can take the right set of items so not to filter items here. It can also accept some "orientation" prop to render itself horizontally or vertically, and possible some "stacked" flag to turn stacking behaviour on or off if that's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Andrei! Thanks for reviewing the changes so quickly! I agree, I guess I did it this way in order to not cause too many rippling changes in the code. Now that I have your opinion I'll refactor it to make it simpler!
src/core/chart-core.tsx
Outdated
| ) : null | ||
| } | ||
| secondLegend={ | ||
| legendOptions && legendOptions.type === "dual" ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we're missing the context.legendEnabled check here
|
|
||
| const series: Highcharts.SeriesOptionsType[] = []; | ||
|
|
||
| Object.entries(primarySeriesData).forEach(([, data], index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks:
- If only using the value and not the key, you can use
Object.valuesinstead ofObject.entries - This could use
mapand this way initialize the valueserieson declaration and avoid mutating it
Something like:
const series = [
...Object.values(primarySeriesData),
...Object.values(secondarySeriesData)
].map((data, index) => ({
name: data[0].name,
type: "line",
data: data,
yAxis: 0,
color: colors[index],
})
);
src/core/chart-core.tsx
Outdated
| type={getLegendType(legendOptions, true)} | ||
| api={api} | ||
| i18nStrings={i18nStrings} | ||
| title={legendOptions.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both legends use the same title and actions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Joan! Thanks for catching this! The titles should indeed be different. For actions however, we're not yet allowing a different set of actions to be displayed. Since this is something we wouldn't require I decided to not implement it, let me know if I should also split it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say better to support a distinct action for the secondary legend right away, otherwise if we add it in the future there is a high chance of breaking the current behavior.
src/core/styles.scss
Outdated
|
|
||
| .bottom-legend-container { | ||
| display: flex; | ||
| overflow: hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is overflow hidden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for hiding the elements when there's no other option than to overflow out of the container. You can see this happening for max heights < 30px, in the example page I put together. Let me know if there's a better way to handle this pls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do overflow: auto then so that overflowing content can be accessible by scrolling?
| .side-legend-container { | ||
| flex: 0; | ||
| overflow-y: auto; | ||
| display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is flex layout necessary if stacking vertically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used display: flex here for forcing overflow in the children instead of in the parent container. In other words, when rendering dual side legends each legend group will get it's independent scroll instead of a global one.
I actually saw there's a max-height: stretch that I could set in the children lists to cause overflow but I believe it's not yet supported in Firefox. Let me know if there's a better way of doing this.
src/core/chart-container.tsx
Outdated
| filter?: React.ReactNode; | ||
| navigator?: React.ReactNode; | ||
| legend?: React.ReactNode; | ||
| firstLegend?: React.ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in other places the second/secondary legend is referred to as "opposite" if I understand correctly, so let's try and unify the naming
8f92e80 to
bd169c5
Compare
|
Note that there are failing tests; see the checks above. |
src/core/utils.ts
Outdated
| markerType: ChartSeriesMarkerType; | ||
| color: string; | ||
| visible: boolean; | ||
| oppositeAxis: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, can we call this something like isSecondary instead?
src/core/chart-core.tsx
Outdated
| type={getLegendType(legendOptions, true)} | ||
| api={api} | ||
| i18nStrings={i18nStrings} | ||
| title={legendOptions.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say better to support a distinct action for the secondary legend right away, otherwise if we add it in the future there is a high chance of breaking the current behavior.
src/core/styles.scss
Outdated
|
|
||
| .bottom-legend-container { | ||
| display: flex; | ||
| overflow: hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do overflow: auto then so that overflowing content can be accessible by scrolling?
001a770 to
2ac9270
Compare
| act(async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| }); | ||
| const mouseLeavePause = () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this change fixed some of the tests for me
src/core/chart-container.tsx
Outdated
| <div className={styles["side-legend-container"]} style={{ maxBlockSize: effectiveChartHeight }}> | ||
| {legend} | ||
| <div | ||
| aria-label={"Legend"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot hardcode ARIA labels like this, and why do we need it in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I left it hardcoded because I wasn't sure how to properly handle it. I completely forgot I left it there. I remember we discussed that the legends container should have an aria-label too, is this what we want still?
I see we grab the legend's aria label with const ariaLabel = i18n("i18nStrings.legendAriaLabel", i18nStrings?.legendAriaLabel); in src/core/components/core-legend.tsx. So I guess we need to add a couple of keys to the i18nStrings:
- singleLegendAriaLabel
- primaryLegendAriaLabel
- secondaryLegendAriaLabel
- bottomLegendContainerAriaLabel
- sideLegendContainerAriaLabel
Please tell me how to proceed with this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove aria-label from here for now
src/core/chart-core.tsx
Outdated
| {...legendOptions} | ||
| position={legendPosition} | ||
| api={api} | ||
| type={legendOptions?.type === "dual" ? "primary" : "single"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need legendOptions.type? Can we infer this from the axis configuration: if there are secondary items -> then we need two legends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong but if I recall correctly we wanted the dual axis splitting as an optional feature and not solely based on whether there's secondary items or not. What I think may be missing here is the check for secondary items as well.
| }); | ||
| }); | ||
|
|
||
| test("toggles series visibility by clicking on legend", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we need to create a series of unit tests to validate the secondary legend, so that:
- We ensure there are test utils to find the and assert the secondary legend items;
- We secure regression for the secondary legend to be rendered and have expected items;
- We secure regression for the secondary legend actions to trigger even handlers expectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do this after we're happy with the interfaces
| }); | ||
| } | ||
| // Needed for touch devices. | ||
| fireNonCancelableEvent(onClearHighlight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to call this in the core legend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean just the fireNonCancelableEvent? These functions were moved up because now the items that the legend takes may be already filtered. However, for emitting onToggleItem and onSelectItem I still need access to the full list of items. I can also pass the full list of items as an extra prop such that we can move these back inside the legend component, let me know if that's what you want.
| }: ChartLegendProps) => { | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const [shouldAutoStack, containerQueryRef] = useContainerQuery((rect) => rect.borderBoxWidth <= SHOULD_STACK_SIZE); | ||
| const containerRef = containerQueryRef as React.RefObject<HTMLDivElement>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not repurpose refs. The conventional way for this is:
const containerObjectRef = useRef()
const containerRef = useMergeRefs(containerQueryRef, containerObjectRef)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Thanks
| someHighlighted: boolean; | ||
| items: readonly LegendItem[]; | ||
| alignment: "horizontal" | "vertical"; | ||
| type: "single" | "primary" | "secondary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this component interface more abstract. E.g. the type can probably be replaced with contentAlignment, and the container query can probably go to the chart-container, which defines most of the layout. Then, we can pass alignment as "vertical" when detecting that stacking behaviour is needed, instead of doing this: const isStacked = alignment === "vertical" || shouldAutoStack;.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is:
- src/core/chart-container.tsx - determines the layout
- src/core/components/core-legend.tsx - defines the data
- src/internal/components/chart-legend - renders stuff
API-wise, the container slots (primaryLegend and secondaryLegend) can be render-props from (stacked):
primaryLegend={(stacked) => <ChartLegend stacked={stacked} ... />}
Inside the container, we don't need two container queries - one is enough (so breaking at 800px instead of 400px).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR to follow this, with a single container query. Let me know what you think of it
c09e973 to
98fc6bf
Compare
src/core/chart-container.tsx
Outdated
| }: ChartContainerProps) { | ||
| const { refs, measures } = useContainerQueries(); | ||
|
|
||
| const showBottomLegend = primaryLegend !== undefined && legendPosition === "bottom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove stacking behaviour for now
src/core/utils.ts
Outdated
| } | ||
|
|
||
| export function hasVisibleSecondaryLegendItems(options: Highcharts.Options) { | ||
| const yAxes = Array.isArray(options.yAxis) ? options.yAxis : options.yAxis ? [options.yAxis] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add more code comments to explain why do we target yAxis only, and that we only support two axes at most.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I think this specific line does not really require much explanation - but feel free to leave the comments if you think it improves code readability.
src/core/utils.ts
Outdated
| export function hasVisibleSecondaryLegendItems(options: Highcharts.Options) { | ||
| const yAxes = Array.isArray(options.yAxis) ? options.yAxis : options.yAxis ? [options.yAxis] : []; | ||
| return !!options.series?.some((series) => { | ||
| // Pie and errorbar series are skipped completely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gauge series?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update: remove the if?
| * This property only applies when `position="bottom"`. | ||
| * This property only applies when `type="dual"`. | ||
| */ | ||
| secondaryLegendTitle?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is no visible label - we pass i18nStrings.legendAriaLabel as ARIA label of the legend container. We must ensure that is also supported for secondary.
Unless.. there is a requirement to always have visible labels when there are more than one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with not requiring visible labels then.
| someHighlighted: boolean; | ||
| items: readonly LegendItem[]; | ||
| alignment: "horizontal" | "vertical"; | ||
| contentAlignment: "single" | "primary" | "secondary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"start" | "center" | "end"
| } | ||
| }, SCROLL_DELAY); | ||
| }, [items, scrollIntoViewControl]); | ||
| }, [containerRef, items, scrollIntoViewControl]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the containerRef can be removed
| return; | ||
| } | ||
| const container = containerRef.current; | ||
| const container = containerRef?.current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const container = containerRef?.current; | |
| const container = containerRef.current; |
be0e3ca to
5fe2f67
Compare
src/core/components/core-legend.tsx
Outdated
| const ariaLabel = i18n( | ||
| "i18nStrings.legendAriaLabel", | ||
| items === "primary" ? i18nStrings?.legendAriaLabel : i18nStrings?.secondaryLegendAriaLabel, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. Should it still be 'i18nString.legendAriaLabel'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must have two separate labels:
const legendAriaLabel = i18n('i18nStrings.legendAriaLabel', i18nStrings?.legendAriaLabel);
const secondaryLegendAriaLabel = i18n('i18nStrings.secondaryLegendAriaLabel', i18nStrings?.secondaryLegendAriaLabel);
const ariaLabel = isSecondary ? secondaryLegendAriaLabel : legendAriaLabel;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we need an update in @cloudscape-design/components so we can consume it here, right? I'll commit this change anyway even though it will fail the build due to typing errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. Yes - we need a change to components to provide the new label for secondary legend.
For the initial release, we can then do the following (with a code comment):
const legendAriaLabel = i18n('i18nStrings.legendAriaLabel', i18nStrings?.legendAriaLabel);
const secondaryLegendAriaLabel = i18nStrings?.secondaryLegendAriaLabel;
const ariaLabel = isSecondary ? secondaryLegendAriaLabel : legendAriaLabel;
That means, that only the primary legend has built-in i18n support, while the secondary is pretty much required. That implies the following guideline to follow: when using dual axes, explicitly provide visible titles or ARIA labels for both primary and secondary legend.
|
|
||
| test("getSecondaryLegendItems returns empty array when no secondary legend exists", () => { | ||
| renderChart({ highcharts, options: { series: primarySeries } }); | ||
| expect(createChartWrapper().findSecondaryLegend()?.findItems() ?? []).toEqual([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally use !.findItems() instead of ?.findItems(). This way, when it fails - this ensures that this code fails if the legend is undefined (unexpected). Also when comparing against non-empty array, the error is going to be "cant call findItems on undefined" instead or array mismatch error, which is less telling in that case.
| expect(createChartWrapper().findSecondaryLegend()).toBe(null); | ||
| }); | ||
|
|
||
| test("renders secondary legend when secondary axis series exist", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that different from the "findSecondaryLegend returns secondary legend when secondary axis series exist" test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, sorry for missing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wories
| test("findSecondaryLegend returns secondary legend when secondary axis series exist", () => { | ||
| renderChart({ | ||
| highcharts, | ||
| options: { series: [...primarySeries, ...secondarySeries], yAxis: yAxes }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when only secondary items exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- when there are to axes
- when there is only one axis and it is secondary
It is important to get this covered and also ensure that this API cannot be abused to force the chart's only legend to be placed on the right hand side, to compensate for the missing alignment option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution I arrived to address this is to not render the secondary legend if there's no primary items but there's secondary items. In this case the secondary items get rendered through the primary legend.
The other option would be to always render the secondary items through the secondary legend but conditionally setting the alignment to start or end depending on whether there's primary items to display. I went with the first approach because testing for the secondary legend to not exist seemed better than testing if the right class is applied to the secondary legend.
However, I also see that doing this may cause some confusion: the primary legend is used to render the primary items unless the only item group is the secondary one, in which case it renders this one.
Tell me what do you think of both approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is best to always use primary legend for primary items and secondary legend for secondary items (that means that the test classes are attached accordingly, so if there are only secondary items - the will be a secondary legend, but no primary legend).
When it comes to alignment, I think that it is actually less important probably as this combination should not be used anyways. Let's do whatever is easier to maintain then.
|
|
||
| const secondaryLegend = createChartWrapper().findSecondaryLegend(); | ||
| expect(secondaryLegend).not.toBe(null); | ||
| const title = secondaryLegend!.find('[class*="title"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the test-utils method to get the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the actions tests below
| data: [], | ||
| }, | ||
| ], | ||
| } as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid using any. Would the same work when using as const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to use real Highcharts chart instances, hope this is okay. There was no way to properly type the chart variable without setting ~40 missing properties.
src/core/utils.ts
Outdated
| // Handles three cases: array of axes, single axis object, or no axes | ||
| const yAxes = Array.isArray(options.yAxis) ? options.yAxis : options.yAxis ? [options.yAxis] : []; | ||
|
|
||
| return !!options.series?.some((series) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good comments here 👍
src/core/utils.ts
Outdated
| // Check if this y-axis is a secondary axis (positioned on the opposite side of the chart) | ||
| const isSecondary = yAxis?.opposite ?? false; | ||
|
|
||
| // Return true if this series is on a secondary axis and should be shown in the legend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this particular comment can be removed as the line is self-explanatory
| items, | ||
| title, | ||
| actions, | ||
| ariaLabel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that for some reason we do not use the built-in i18n for ARIA label for this legend component. Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I should fix that! I'll address that with this PR if that's okay.
| ); | ||
| }; | ||
|
|
||
| function getLegendTitleAlignment(horizontalAlignment: "start" | "center" | "end") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would call this "getLegendToBoxAlignment" or something like that. This can be reinforced if using explicit types for input/output:
function getBoxTextAlignment(legendAlignment: LegendAlignment): Box.TextAlignment {/*...*/}
src/test-utils/dom/internal/core.ts
Outdated
| } | ||
|
|
||
| public findSecondaryLegend(): null | CoreChartLegendWrapper { | ||
| return this.findComponent(`.${CoreChartLegendWrapper.rootSelector}:nth-child(2)`, CoreChartLegendWrapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to introduce another test class and pass it to the secondary legend component. In that case there will be no need to rely on the nth-child(2) which is prone to future refactoring if the legend layout will get more complex so that the legend items are no longer siblings.
src/core/components/core-legend.tsx
Outdated
| position: "bottom" | "side"; | ||
| horizontalAlignment?: CoreChartProps.LegendOptionsHorizontalAlignment; | ||
| alignment: "horizontal" | "vertical"; | ||
| type: "single" | "primary" | "secondary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added single here for grabbing all the items. In practice it never displays both primary and secondary items together due to the way the chart core is calling it. It's used as 'grab whatever items there are'
src/core/components/core-legend.tsx
Outdated
| const i18n = useInternalI18n("[charts]"); | ||
| const ariaLabel = i18n("i18nStrings.legendAriaLabel", i18nStrings?.legendAriaLabel); | ||
| const legendAriaLabel = i18n("i18nStrings.legendAriaLabel", i18nStrings?.legendAriaLabel); | ||
| const secondaryLegendAriaLabel = i18n("i18nStrings.legendAriaLabel", i18nStrings?.secondaryLegendAriaLabel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't write "i18nStrings.secondaryLegendAriaLabel" yet because that will trigger a typing error. I think we need an update on the components package.
src/core/chart-core.tsx
Outdated
| } | ||
| secondaryLegend={ | ||
| context.legendEnabled && legendProps.secondary ? ( | ||
| <ChartLegend {...commonLegendProps} {...legendProps.secondary} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also extracted common props into commonLegendProps, hope it's okay.
| * @i18n | ||
| */ | ||
| i18nStrings?: CartesianI18nStrings & PieI18nStrings; | ||
| i18nStrings?: CartesianI18nStrings & PieI18nStrings & CoreI18nStrings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the new CoreI18nStrings to the intersection but probably what we want to do is to make CartesianI18nStrings and PieI18nStrings extend CoreI18nStrings instead of BaseI18nStrings?
Edit: Doing this would leak the internal core prop so I guess this is the correct way to go
src/core/utils.ts
Outdated
| // - One group for all axes with opposite=false (primary axes) | ||
| // - One group for all axes with opposite=true (secondary axes) | ||
| // | ||
| // Important: A secondary legend should only be shown when BOTH primary and secondary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting approach - I don't mind that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From your previous comments:
It is important to get this covered and also ensure that this API cannot be abused to force the chart's only legend to be placed on the right hand side, to compensate for the missing alignment option.
I think it is best to always use primary legend for primary items and secondary legend for secondary items (that means that the test classes are attached accordingly, so if there are only secondary items - the will be a secondary legend, but no primary legend).
When it comes to alignment, I think that it is actually less important probably as this combination should not be used anyways. Let's do whatever is easier to maintain then.
I may be misinterpreting this but it seems (1) and (2) are contradicting each other. Regardless of that, I will update the code to always use primary legend for primary items and secondary legend for secondary items.
If there's only secondary series to be displayed, the secondary legend will display them aligned to the right.
src/core/utils.ts
Outdated
| // Why we target yAxis only: | ||
| // In typical chart usage, dual-axis layouts use the y-axis for different scales | ||
| // (e.g., temperature on left, humidity on right). The x-axis is shared across all series. | ||
| // While Highcharts technically supports opposite x-axes, this is rare in practice and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that is actually interesting. I think what you are referring to is called chart.inverted. Can we do some isInverted ? xAxis : yAxis instead? It feels that supporting the inverted orientation should not be hard.
The opposite is also interesting, though. It allow to render the axis to the opposite side of the chart, which should mean that the primary and secondary legends should exchange places. I think it is fine to not support it with the initial implementation, but let's document in comments properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually not referring to chart.inverted tbh. Highcharts does actually support dual x-axes configurations. But your point does make sense, I'll update this code to take that into account!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the second part of your comment, I'm not entirely sure which prop controls that behavior. I see there's axis.reversed but that will only reverse the ticks of that axis.
9e411b4 to
69b20fd
Compare
80443d9
Add dual axis support in legend
Implements dual axis support for chart legends in the Cloudscape chart components, allowing legends to properly display and differentiate between series that belong to different Y-axes.
Changes
• Core Legend Enhancement: Updated legend components to handle dual axis scenarios with proper visual differentiation
• Chart Container Updates: Modified chart container logic to support dual axis legend rendering
• Interface Extensions: Added new interface properties to support dual axis configuration
• Styling Improvements: Updated SCSS styles to accommodate dual axis legend layouts
• Demo Page: Added comprehensive dual axis chart demo page with examples
Single Legend Bottom
Dual Legend Bottom
Single Legend Side
Dual Legend Side