Update Alerts, Default Keys, Update Columns #2461
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements 508 compliance features for confidence interval columns by automatically adding them to tooltips and data tables, and warning users when trying to hide these required columns. Additionally, it enhances the Alert component to support custom font sizes.
- Auto-adds confidence key columns to tooltips and data table when selected
- Adds warning alert for confidence interval columns in the data table editor
- Enhances Alert component with customizable font size
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/components/EditorPanel/ColumnsEditor.tsx | Adds 508 compliance warning alert when confidence interval columns are being configured |
| packages/core/components/Alert/components/Alert.tsx | Adds fontSize prop to Alert component for customizable text size |
| packages/chart/src/components/EditorPanel/EditorPanel.tsx | Auto-adds confidence key columns with default settings when selected, and formats legend position options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [columnName]: existingColumn | ||
| ? { ...existingColumn, dataTable: true, tooltips: true } | ||
| : { | ||
| name: columnName, | ||
| label: columnName, | ||
| dataTable: true, | ||
| tooltips: true, | ||
| prefix: '', | ||
| suffix: '', | ||
| forestPlot: false, | ||
| startingPoint: '0', | ||
| forestPlotAlignRight: false, | ||
| roundToPlace: 0, | ||
| commas: false, | ||
| showInViz: false, | ||
| forestPlotStartingPoint: 0 | ||
| } |
There was a problem hiding this comment.
The default column configuration object is duplicated in this code. Consider extracting it into a constant or factory function to avoid duplication and ensure consistency with other column creation logic in the codebase.
Co-authored-by: Copilot <[email protected]>
| {type === 'danger' && <Icon display='warningCircle' size={iconSize} />} | ||
| {type === 'info' && <Icon display='info' size={iconSize} />} | ||
| <span dangerouslySetInnerHTML={sanitizedData()} /> | ||
| <span dangerouslySetInnerHTML={sanitizedData()} style={fontSize ? { fontSize } : undefined} /> |
There was a problem hiding this comment.
For standardization purposes, I would force the font size instead of making it a user configurable option.
adamdoe
left a comment
adamdoe
left a comment
There was a problem hiding this comment.
One thing to address, the font size setting should either be removed or we should get feedback on it for standardization. Otherwise, this is looking great!
Summary