feat: add flag conditional formating cell bars #35897
Conversation
This reverts commit 35aaeb6.
Code Review Agent Run #f31e6cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
bot
added
change:frontend
![korbit-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inconsistent Property Naming Pattern ▹ view | |
 |
Missing memoization for columnTypeNumeric computation ▹ view | ✅ Fix detected |
 |
Inefficient conditional property assignment ▹ view | ✅ Fix detected |
|
Inconsistent showCellBars condition between formatter and renderer ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts | ✅ |
| superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| export type ConditionalFormattingFlag = { | ||
| toAllRowCheck?: boolean; | ||
| toColorTextCheck?: boolean; | ||
| toCellBarCheck?: boolean; | ||
| }; |
There was a problem hiding this comment.
Inconsistent Property Naming Pattern
Tell me more
What is the issue?
The naming convention between ConditionalFormattingConfig and ConditionalFormattingFlag is inconsistent. The Config uses 'toAllRow', 'toTextColor', while the Flag uses 'toAllRowCheck', 'toColorTextCheck'.
Why this matters
Inconsistent naming patterns increase cognitive load and make the codebase harder to maintain. It also violates the principle of least surprise.
Suggested change ∙ Feature Preview
Align the naming patterns either by using the 'Check' suffix consistently or removing it entirely. Example:
export type ConditionalFormattingFlag = {
toAllRow?: boolean;
toTextColor?: boolean;
toCellBar?: boolean;
};Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
ConditionalFormattingFlag are the types of values that are passed from controlPanel.
There was a problem hiding this comment.
I see, thanks for clarifying. Is there a specific reason for using different naming conventions between ConditionalFormattingConfig and ConditionalFormattingFlag? If not, would aligning them improve consistency?
...nd/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx
Outdated
Show resolved
Hide resolved
Interaction Diagram by BitosequenceDiagram
participant User as User<br/>
participant UI as FormattingPopoverContent<br/>🔄 Updated | ●●● High
participant Panel as ControlPanel<br/>🔄 Updated | ●●○ Medium
participant Formatter as getColorFormatters<br/>🔄 Updated | ●●○ Medium
participant Chart as TableChart<br/>🔄 Updated | ●●● High
participant Types as Type Definitions<br/>🔄 Updated | ●●○ Medium
User->>UI: Configure conditional formatting
UI->>UI: Set toCellBar checkbox state
UI->>Panel: Submit formatting config with toCellBar
Panel->>Types: Use ConditionalFormattingConfig type
Panel->>Formatter: Pass config to getColorFormatters
Formatter->>Formatter: Create ColorFormatters with toCellBar property
Formatter-->>Chart: Return formatters array
Chart->>Chart: Apply toCellBar formatter to cell background
Chart-->>User: Render table with conditional cell bar colors
Critical path: User->FormattingPopoverContent->ControlPanel->getColorFormatters->TableChart
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for conditional formatting with cell bars in table charts. The feature allows users to apply conditional color formatting specifically to cell bars (background bars that indicate values) for numeric columns, independent of text color and background color formatting.
- Adds
toCellBarboolean flag to conditional formatting configuration types - Implements UI controls for the new cell bar formatting option with mutual exclusivity from text/background formatting
- Updates table rendering logic to apply conditional colors to cell bars
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types.ts (explore/controls) | Added toCellBar property to ConditionalFormattingConfig and ConditionalFormattingFlag types |
| FormattingPopoverContent.tsx | Added cell bar checkbox UI with logic to disable conflicting options |
| FormattingPopoverContent.test.tsx | Updated tests to verify cell bar option appears for numeric columns only |
| controlPanel.tsx | Added initialization logic for toCellBar flag in control panel configuration |
| TableChart.tsx | Implemented cell bar color application from conditional formatting |
| getColorFormatters.ts | Added toCellBar to color formatter configuration |
| types.ts (chart-controls) | Added toCellBar property to shared types |
| }); | ||
|
|
||
| test('displays the toAllRow and toTextColor flags based on the selected numeric type operator', () => { | ||
| test('displays the toAllRow, toTextColor and toCellBar flags based on the selected numeric type operator', () => { |
There was a problem hiding this comment.
Extra space in 'toTextColor and'. Should be 'toTextColor and'.
| test('displays the toAllRow, toTextColor and toCellBar flags based on the selected numeric type operator', () => { | |
| test('displays the toAllRow, toTextColor and toCellBar flags based on the selected numeric type operator', () => { |
| }); | ||
|
|
||
| test('Not displays the toAllRow and toTextColor flags', () => { | ||
| test('Not displays the toAllRow, toTextColor and toCellBar flags', () => { |
There was a problem hiding this comment.
Grammatically incorrect test name. Should be 'Does not display' instead of 'Not displays'. Also has extra spaces before 'and' and after 'toCellBar'.
| test('Not displays the toAllRow, toTextColor and toCellBar flags', () => { | |
| test('Does not display the toAllRow, toTextColor and toCellBar flags', () => { |
1 participant
This PR is a continuation #34762 of
to expand the possibilities of conditional formatting
SUMMARY
Add flag toCellBar - apply formatting to cell bars
Cell bar formatting is applied when "Show cell bars" is enabled.
Implements the proposal mentioned in #32250
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
run test src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx
ADDITIONAL INFORMATION