-
Notifications
You must be signed in to change notification settings - Fork 9
feat: dual axis legend #108
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
|
@jsilll some of the existing pages are failing with errors, e.g. the |
|
@jsilll, that is not directly related, but should "Legend bottom max height" include legend's title and the margin between title and items? Currently, only the items are included. Edit: it turns out, the question was related. In the current implementation, the entire legend is covered by the max height setting. |
| items={legendItems} | ||
| actions={actions} | ||
| position={position} | ||
| bottomMaxHeight={bottomMaxHeight} |
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 implement the legend layout inside the legend itself? Instead of passing the bottomMaxHeight down to the legend, you can give extra slot in the chart-container, so that it can render either a single or a double legend.
| return null; | ||
| } | ||
|
|
||
| // TODO: surface the type and defaultTitle and oppositeTitle props |
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.
should this comment be removed or is there something still planned?
| const resizeObserver = new ResizeObserver((entries) => { | ||
| if (entries.length > 0) { | ||
| const width = entries[0].borderBoxSize?.[0].inlineSize; | ||
| setShouldStack(width < 400); |
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 id it 400px? Let's extract this into a constant
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, 400 was simply my choice for the threshold, let's agree on a sensible value.
| if (!containerRef.current) { | ||
| return; | ||
| } | ||
| const resizeObserver = new ResizeObserver((entries) => { |
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.
CDS has useContainerQuery and useResizeObserver utils that can be used instead of this custom code.
| tabIndex={-1} | ||
| ref={containerRef} | ||
| style={{ | ||
| overflow: "auto", |
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 inline styles can only be used when the values are computed dynamically. The overflow: auto should be added via classes instead
| } | ||
|
|
||
| .legend-dual-group { | ||
| inline-size: calc(50% - 20px); |
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 add a comment explaining the values here. Or, better, you can extract the 20px (or 40px) into a variable. E.g. some $legend-horizontal-padding: 40px, and then inline-size: calc(50% - $legend-horizontal-padding / 2)

Description
Introduces support for dual-axis chart legends, which properly groups and separates legend items based on which Y-axis they belong to.
Key changes:
oppositeAxisproperty to legend items to track which Y-axis a series belongs to