Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes button usage across editor surfaces by expanding the shared @cdc/core Button API (variants/sizes/full-width), introduces a reusable GroupedList wrapper for editor “series-style” lists (optionally draggable via @hello-pangea/dnd), and updates multiple chart/map/dashboard/editor panels + styles to align with the new patterns.
Changes:
- Added
docs/BUTTON_SYSTEM.mdand updatedAGENTS.mdto document Button migration rules and exceptions. - Expanded the shared
Buttoncomponent + styles to support semanticvariant,size, andfullWidthprops (and migrated many call sites). - Added
GroupedList(and Storybook) and refactored several panels to use grouped/accordion list rendering (with drag-and-drop reordering in some panels).
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/waffle-chart/src/CdcWaffleChart.tsx | Migrates “I’m Done” action to shared Button. |
| packages/map/src/components/EditorPanel/components/Panels/Panel.PatternSettings.tsx | Refactors pattern editor rows into GroupedList + shared Button. |
| packages/map/src/components/EditorPanel/components/Panels/Panel.Annotate.tsx | Refactors annotations UI to grouped/accordion layout and shared inputs/buttons. |
| packages/map/src/components/EditorPanel/components/EditorPanel.tsx | Converts multiple “list editors” to GroupedList + shared Button patterns. |
| packages/map/src/components/EditorPanel/components/editorPanel.styles.css | Adds spacing hook for an editor action button. |
| packages/map/src/components/Annotation/AnnotationList.tsx | Aligns context usage to { config }. |
| packages/editor/src/scss/main.scss | Updates legacy .btn hover/active transitions. |
| packages/editor/src/components/PreviewDataTable.tsx | Migrates pagination buttons to shared Button. |
| packages/editor/src/components/modal/Confirmation.jsx | Migrates confirmation modal actions to shared Button. |
| packages/editor/src/components/DataImport/components/DataImport.tsx | Migrates several actions to shared Button (partially via legacy classnames). |
| packages/editor/src/components/ChooseTab.tsx | Migrates “Load” action to shared Button. |
| packages/data-bite/src/components/EditorPanel/EditorPanel.tsx | Migrates editor actions to shared Button props. |
| packages/data-bite/src/CdcDataBite.tsx | Migrates “I’m Done” action to shared Button. |
| packages/dashboard/src/scss/main.scss | Adds layout rules for dashboard filter actions + expand/collapse button styling. |
| packages/dashboard/src/scss/grid.scss | Normalizes dashboard builder button hover/active behavior with shared Button. |
| packages/dashboard/src/components/Widget/Widget.tsx | Migrates widget configure actions to shared Button. |
| packages/dashboard/src/components/Widget/widget.styles.css | Updates widget menu styling to target .cove-button configure buttons. |
| packages/dashboard/src/components/Row.tsx | Migrates row menu/configure controls to shared Button. |
| packages/dashboard/src/components/MultiConfigTabs/MultiConfigTabs.tsx | Migrates modal/tab controls to shared Button where appropriate. |
| packages/dashboard/src/components/Grid.tsx | Migrates “Add Row” to shared Button. |
| packages/dashboard/src/components/ExpandCollapseButtons.tsx | Migrates expand/collapse controls to shared Button. |
| packages/dashboard/src/components/DashboardFilters/DashboardFiltersEditor/DashboardFiltersEditor.tsx | Migrates “Add Filter” buttons to shared Button. |
| packages/dashboard/src/components/DashboardFilters/DashboardFiltersEditor/components/FilterEditor.tsx | Migrates “Edit API Values” to shared Button. |
| packages/dashboard/src/components/DashboardFilters/DashboardFiltersEditor/components/DeleteFilterModal.tsx | Migrates delete-confirm modal actions to shared Button variants. |
| packages/dashboard/src/components/DashboardFilters/DashboardFiltersEditor/components/APIModal.tsx | Migrates modal save action to shared Button. |
| packages/dashboard/src/components/DashboardFilters/DashboardFilters.tsx | Migrates dashboard filter submit/reset actions to shared Button + new actions wrapper. |
| packages/core/styles/themes/_color-definitions.scss | Ensures themed “Apply” buttons style correctly when using shared Button. |
| packages/core/styles/_series-list.scss | Extends series-list action styling to grouped-list remove buttons and panel action row. |
| packages/core/styles/_global.scss | Refines legacy .btn hover/active transitions and avoids effects on .btn-link. |
| packages/core/components/PaletteSelector/DeveloperPaletteRollback.tsx | Migrates rollback button to shared Button. |
| packages/core/components/Filters/Filters.tsx | Migrates “Clear Filters” to shared Button variant='link'. |
| packages/core/components/elements/button.scss | Adds new variant/size/full-width styling + legacy alias support updates. |
| packages/core/components/elements/Button.jsx | Adds variant, size, fullWidth handling and changes click handling behavior. |
| packages/core/components/elements/_stories/Button.stories.tsx | Expands Storybook to document/audit button variants and legacy patterns. |
| packages/core/components/elements/_stories/button-audit.scss | Adds Storybook-only styling for the audit story. |
| packages/core/components/EditorPanel/VizFilterEditor/VizFilterEditor.tsx | Refactors filter editor to GroupedList + accessible accordion + shared buttons. |
| packages/core/components/EditorPanel/GroupedList.tsx | Introduces reusable grouped list wrapper with optional DnD support. |
| packages/core/components/EditorPanel/GroupedList.styles.css | Adds grouped list base styling and panel action alignment. |
| packages/core/components/EditorPanel/FootnotesEditor.tsx | Refactors static footnotes editor to GroupedList with DnD and shared buttons. |
| packages/core/components/EditorPanel/FieldSetWrapper.tsx | Migrates wrapper controls to shared Button variants/sizes. |
| packages/core/components/EditorPanel/components/MarkupVariablesEditor.tsx | Migrates “Add Variable” to shared Button prop API. |
| packages/core/components/EditorPanel/ColumnsEditor.tsx | Refactors columns editor to GroupedList with DnD and shared buttons. |
| packages/core/components/AdvancedEditor/EmbedEditor.tsx | Migrates modal buttons to shared Button variants. |
| packages/core/components/AdvancedEditor/AdvancedEditor.tsx | Migrates “Apply Configuration Changes” to shared Button. |
| packages/core/components/_stories/GroupedList.stories.tsx | Adds Storybook coverage for GroupedList draggable/non-draggable modes. |
| packages/chart/src/components/EditorPanel/components/Panels/Panel.Series.tsx | Migrates series remove/CI add actions to shared Button and aligns panel actions layout. |
| packages/chart/src/components/EditorPanel/components/Panels/Panel.Sankey.tsx | Migrates “Add StoryNode” to shared Button editor-primary. |
| packages/chart/src/components/EditorPanel/components/Panels/Panel.Regions.tsx | Refactors regions editor to GroupedList with DnD and shared button actions. |
| packages/chart/src/components/EditorPanel/components/Panels/Panel.PatternSettings.tsx | Refactors pattern editor to GroupedList with DnD reordering and shared buttons. |
| packages/chart/src/components/EditorPanel/components/Panels/Panel.Annotate.tsx | Refactors annotations editor to GroupedList with DnD reordering and shared buttons. |
| docs/BUTTON_SYSTEM.md | Adds button system/migration documentation. |
| AGENTS.md | Links the new button system doc for contributors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -90,8 +100,12 @@ const Button = ({ | |||
| <button | |||
| {...attributesObj} | |||
| onClick={e => { | |||
| e.preventDefault() | |||
| return loading || onClick(e) | |||
| if (loading) { | |||
| e.preventDefault() | |||
| return true | |||
| } | |||
|
|
|||
| return onClick?.(e) | |||
| }} | |||
| disabled={loading || attributesObj.disabled} | |||
There was a problem hiding this comment.
Button no longer calls preventDefault() for non-loading clicks and does not set a default type. This can introduce accidental form submissions (HTML default is type="submit" when inside a <form>) and is a breaking behavioral change vs. the previous always-preventDefault implementation. Consider setting a safe default type='button' when attributes.type is not provided, and avoid changing submit behavior unless the callsite explicitly opts in with type='submit' (or document the breaking change).
| return ( | ||
| <div className={joinClassNames('grouped-list', className)}> | ||
| {label && <div className='grouped-list__label'>{label}</div>} | ||
| {!draggable && ( | ||
| <ul className={joinClassNames('grouped-list__items', listClassName)} style={listStyle}> | ||
| {items.map((item, index) => renderItem(item, index))} | ||
| </ul> | ||
| )} | ||
| {draggable && ( | ||
| <DragDropContext onDragEnd={onDragEnd || noop}> | ||
| <Droppable droppableId={droppableId}> | ||
| {(provided, snapshot) => ( | ||
| <ul | ||
| {...provided.droppableProps} | ||
| className={joinClassNames( | ||
| 'grouped-list__items', | ||
| snapshot.isDraggingOver ? 'grouped-list__items--drag-over' : undefined, | ||
| listClassName | ||
| )} | ||
| ref={provided.innerRef} | ||
| style={listStyle} | ||
| > | ||
| {items.map((item, index) => renderItem(item, index))} | ||
| {provided.placeholder} | ||
| </ul> | ||
| )} | ||
| </Droppable> |
There was a problem hiding this comment.
GroupedList renders a <ul> but inserts renderItem(...) output directly. Most current call sites return <div> / <Accordion> / <Draggable> wrappers (not <li>), which produces invalid HTML (non-<li> children inside <ul>) and can harm list semantics for accessibility. Suggest wrapping each item in an <li> inside GroupedList (and pass a renderItemContent), or switch the container to a <div> if list semantics aren’t intended.
| <GroupedList | ||
| items={config?.annotations} | ||
| label='Text Annotations' | ||
| droppableId='map-annotations-order' | ||
| draggable={false} | ||
| renderItem={(annotation, index) => ( |
There was a problem hiding this comment.
This panel wraps annotations in GroupedList but sets draggable={false} and does not provide onDragEnd / Draggable items, so drag-and-drop reordering is not actually enabled here. If reordering is intended (per PR description), GroupedList should be draggable and the list items should be wrapped in Draggable with an onDragEnd handler that updates config.annotations order; otherwise the PR description should be adjusted.
| <GroupedList | ||
| items={patterns} | ||
| label='Geo Patterns' | ||
| droppableId='map-geo-patterns' | ||
| draggable={false} | ||
| renderItem={(pattern, patternIndex) => { |
There was a problem hiding this comment.
GroupedList is introduced for patterns but draggable={false} is set and there is no drag end handler, so patterns cannot be reordered via drag-and-drop. If the intention is to support reordering (per PR description), the list should be made draggable with Draggable items and an onDragEnd that updates the patterns order in config.
| @@ -665,7 +666,7 @@ const DataImport = () => { | |||
| }} | |||
| > | |||
| Add New URL Filter | |||
| </button> | |||
| </Button> | |||
There was a problem hiding this comment.
This new <Button> usage still encodes semantics via legacy classes (btn-primary, full-width) instead of the new prop API documented in docs/BUTTON_SYSTEM.md. To keep the migration consistent, prefer variant='primary' (or editor-primary if appropriate) and fullWidth, and reserve className for spacing/layout-only classes.
| @@ -2340,7 +2369,7 @@ const EditorPanel: React.FC<MapEditorPanelProps> = ({ datasets }) => { | |||
| }} | |||
| > | |||
| Add Category | |||
| </button> | |||
| </Button> | |||
There was a problem hiding this comment.
This <Button> still relies on legacy semantic classes (btn-primary, full-width) plus a custom spacing class. To align with the new Button prop API and reduce style coupling, prefer variant='primary' (or editor-primary) and fullWidth, leaving className for layout-only utilities.
| <div className='button-audit__section'> | ||
| <h3 className='button-audit__title'>Filter Action Buttons</h3> | ||
| <p className='button-audit__copy'> | ||
| The shared filter Apply control uses the shared Button component with theme and apply classes layered on top. | ||
| </p> | ||
| <div className='cove-visualization type-map theme-blue button-audit__row'> | ||
| <div className='button-audit__example'> | ||
| <Button {...args} className='theme-blue apply'> | ||
| Apply | ||
| </Button> | ||
| <p className='button-audit__meta'>Used in shared filters. Ref: `packages/core/components/Filters/Filters.tsx`</p> | ||
| </div> | ||
| <div className='button-audit__example'> | ||
| <button className='btn btn-link'>Clear Filters</button> | ||
| <p className='button-audit__meta'> | ||
| Used next to Apply in shared filters and dashboard filters. Ref: | ||
| `packages/core/components/Filters/Filters.tsx`, `packages/dashboard/src/components/DashboardFilters/DashboardFilters.tsx` | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
The Button audit story’s “Filter Action Buttons” section still uses a raw <button className='btn btn-link'>Clear Filters</button> and cites Filters.tsx / DashboardFilters.tsx, but those call sites have been migrated to the shared Button variant='link'. Update this example (or clarify that it is intentionally demonstrating the legacy pattern) so the story doesn’t misdocument current usage.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/core/components/elements/Button.jsx:112
Buttonno longer callspreventDefault()for non-loading clicks, but it also doesn't set a defaulttype. In a form,<button>defaults totype="submit", so anyButtonused without an explicittypecan now start submitting forms unexpectedly. Consider defaultingtypeto'button'(while allowing callers to override with'submit') to avoid subtle behavior changes across the repo.
return (
<button
{...attributesObj}
onClick={e => {
if (loading) {
e.preventDefault()
return true
}
return onClick?.(e)
}}
disabled={loading || attributesObj.disabled}
ref={buttonRef}
>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button className='btn btn-link'>Clear Filters</button> | ||
| <p className='button-audit__meta'> | ||
| Used next to Apply in shared filters and dashboard filters. Ref: |
There was a problem hiding this comment.
This story’s “Clear Filters” example and its reference text still use a raw <button className='btn btn-link'>, but the codebase has been migrated to the shared Button (variant='link') for filter clear actions. Update the example and/or the description so the audit story reflects current usage.
| <button className='btn btn-link'>Clear Filters</button> | |
| <p className='button-audit__meta'> | |
| Used next to Apply in shared filters and dashboard filters. Ref: | |
| <Button {...args} variant='link'> | |
| Clear Filters | |
| </Button> | |
| <p className='button-audit__meta'> | |
| Used next to Apply in shared filters and dashboard filters via the shared Button link variant. Ref: |
| <Button | ||
| type="button" | ||
| className="btn btn-sm btn-outline-warning" | ||
| onClick={handleRollback} | ||
| title={`Rollback to original palette: ${displayPaletteName}`} | ||
| > | ||
| Rollback Palette | ||
| </button> | ||
| </Button> |
There was a problem hiding this comment.
Now that the shared Button supports variant='outline-warning' and size='sm', this call site shouldn’t need legacy semantic classes (btn btn-sm btn-outline-warning) in className. Using variant/size props here would align with the migration rules in docs/BUTTON_SYSTEM.md and reduce dependence on compatibility selectors.
| <Button | ||
| className={'btn btn-primary full-width editor-panel-action-button'} | ||
| onClick={event => { |
There was a problem hiding this comment.
This Button usage still encodes semantics via legacy Bootstrap classes (btn btn-primary full-width) instead of the new variant/fullWidth props. Since this PR is standardizing Button usage, consider switching to variant='primary' + fullWidth and keeping only the non-semantic layout hook (editor-panel-action-button) in className.
| <AccordionItemButton | ||
| className={ | ||
| chartsWithOptions.includes(config.visualizationType) | ||
| ? 'accordion__button' | ||
| : 'accordion__button hide-arrow' | ||
| } | ||
| > | ||
| <Icon display='move' size={15} style={{ cursor: 'default' }} /> | ||
| {series.dataKey} | ||
| <Series.Button.Remove series={series} index={i} /> | ||
| </AccordionItemButton> | ||
| </AccordionItemHeading> | ||
| {chartsWithOptions.includes(config.visualizationType) && ( | ||
| <AccordionItemPanel> | ||
| <div className='series-item__panel-actions'> | ||
| <Series.Button.Remove series={series} index={i} /> | ||
| </div> |
There was a problem hiding this comment.
The “Remove” series action is now rendered only inside AccordionItemPanel, which is gated by chartsWithOptions.includes(config.visualizationType). For chart types that don’t render the panel, this makes it impossible to remove a series (even when config.series.length > 1). Consider rendering the remove control outside that conditional (e.g., in the header actions area) or decoupling removal from chartsWithOptions so series can always be removed when allowed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces significant improvements to the chart editor panels by standardizing button usage, implementing drag-and-drop reordering for annotations and patterns, and enhancing code consistency. The main themes are: (1) migration to the shared
Buttoncomponent API, (2) addition of accessible drag-and-drop reordering for annotations and patterns, and (3) improved documentation for button usage and migration.Button System Documentation and Migration:
docs/BUTTON_SYSTEM.mddetailing the sharedButtonAPI, migration rules, current status, and exceptions, and referenced it inAGENTS.mdfor relevant contributors. [1] [2]Panel.Annotate.tsxandPanel.PatternSettings.tsxto use the sharedButtoncomponent with semantic props (e.g.,variant,size,fullWidth), removing direct Bootstrap-style class usage for improved consistency and maintainability. [1] [2]Drag-and-Drop Reordering Enhancements:
Panel.Annotate.tsxusing theGroupedListcomponent and@hello-pangea/dnd, allowing users to reorder annotations visually and updating configuration accordingly. [1] [2]Panel.PatternSettings.tsx, enabling users to rearrange patterns and reflecting changes in the chart configuration. [1] [2]Preparatory Changes for Consistency:
Panel.Regions.tsxto prepare for consistent usage of drag-and-drop and shared components, aligning with the changes made in other panels.These changes collectively improve the user experience in the chart editor, streamline future maintenance, and provide clear guidance for ongoing migration work.