Charting tool enhancements - Errors bars for Bar and Line chart#1869
Charting tool enhancements - Errors bars for Bar and Line chart#1869
Conversation
…that is a select option target
…thod dropdown into tooltip
…uded in axis options overlay
# Conflicts: # packages/components/package-lock.json # packages/components/package.json # packages/components/releaseNotes/components.md
There was a problem hiding this comment.
Pull Request Overview
Adds error bar support for bar and line charts in the charting tool. The feature allows users to display standard deviation or standard error of the mean as error bars when using the "Mean" aggregate method for chart data.
- Extracted chart field options components for better separation of concerns
- Added error bar configuration UI with radio button options for Standard Deviation and Standard Error of the Mean
- Extended chart configuration to include error bar settings in the data model
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/components/src/theme/charts.scss | Adds styling for block-layout radio button groups |
| packages/components/src/internal/components/forms/input/RadioGroupInput.tsx | Adds disabled option support and safer key handling |
| packages/components/src/internal/components/chart/utils.ts | Adds utility functions to determine when to show chart options |
| packages/components/src/internal/components/chart/utils.test.ts | Tests for new utility functions |
| packages/components/src/internal/components/chart/models.ts | Defines TypeScript interfaces for chart configuration |
| packages/components/src/internal/components/chart/constants.ts | Centralizes chart-related constants |
| packages/components/src/internal/components/chart/ChartFieldRangeScaleOptions.tsx | Extracted component for scale/range options |
| packages/components/src/internal/components/chart/ChartFieldOption.tsx | Refactored to use extracted components and support error bars |
| packages/components/src/internal/components/chart/ChartFieldOption.test.tsx | Updated tests for component changes |
| packages/components/src/internal/components/chart/ChartFieldAggregateOptions.tsx | New component for aggregate method and error bar selection |
| packages/components/src/internal/components/chart/ChartBuilderModal.tsx | Integrates error bar support into chart configuration |
| packages/components/src/internal/components/chart/ChartBuilderModal.test.tsx | Updated tests for error bar functionality |
| packages/components/src/internal/OverlayTrigger.tsx | Fixes overlay closing when clicking select options |
| packages/components/releaseNotes/components.md | Documents the new features |
| packages/components/package.json | Updates version number |
Files not reviewed (1)
- packages/components/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/components/src/internal/components/forms/input/RadioGroupInput.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/internal/components/chart/ChartFieldRangeScaleOptions.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/internal/components/chart/ChartBuilderModal.test.tsx
Show resolved
Hide resolved
packages/components/src/internal/components/chart/ChartBuilderModal.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // handle bar chart aggregate method | ||
| // handle bar chart aggregate method and error bars |
There was a problem hiding this comment.
I know you're only adding a little bit of logic to this useEffect, but it should not be a useEffect. Instead this should be a useMemo. Nothing in this useEffect is loading anything.
There was a problem hiding this comment.
I assume this was using useEffect because it had a bunch of calls to setX() in the body. I've refactored those to useMemo and/or moved the initial values up to the declaration of the setter.
packages/components/src/internal/components/chart/ChartFieldAggregateOptions.test.tsx
Show resolved
Hide resolved
| if (option.value === '' && !includeNone) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
| if (option.value === '' && !includeNone) { | |
| return false; | |
| } | |
| return true; | |
| return !(option.value === '' && !includeNone); |
There was a problem hiding this comment.
I saw that suggestion from IntelliJ as well. I made the change, but I do find the multiple if statements version more readable (which is why I had it that way)
| scale.min !== null && | ||
| scale.max !== undefined && | ||
| scale.max !== null && | ||
| parseFloat(scale.max.toString()) <= parseFloat(scale.min.toString()), |
There was a problem hiding this comment.
Why do we need to do parseFloat(scale.max.toString())? This feels odd.
There was a problem hiding this comment.
because without that we are comparing strings and a max value of '20' <= a min value of '3 would give and invalid range message
packages/components/src/internal/components/chart/ChartFieldRangeScaleOptions.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/internal/components/forms/input/RadioGroupInput.tsx
Outdated
Show resolved
Hide resolved
| const insideToggle = portalEl?.contains(event.target); | ||
| if (!isToggle && !insideToggle) { | ||
| const isSelectOption = | ||
| event.target instanceof HTMLElement && event.target.classList.contains('select-input__option'); |
| ] | ||
| [savedChartModel, chartConfig, TRENDLINE_OPTIONS] | ||
| ); | ||
| const [fieldValues, setFieldValues] = useState<Record<string, SelectInputOption>>(initFieldValues); |
There was a problem hiding this comment.
Instead of using useMemo you can pass the same method you used for useMemo to useState, useState accepts function initalizers that will run on mount only. Documented here.
2 participants
Rationale
We have supported rendering error bars in our vis package for awhile now for Study Time Charts to render the SD or SEM values for mean aggregates when rendering time charts of group data. This PR updates the charting UI and rendering to allow for error bars to be shown for mean aggregates for Bar and Line Charts as well. The options in the UI allow for errors bars to be calculated and displayed as standard deviation OR standard error of the mean. Note that this is only applicable when the aggregate method is selected as "Mean".
Related Pull Requests
Changes