-
Notifications
You must be signed in to change notification settings - Fork 16.3k
fix(theming): fix TimeTable chart issues #34868
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
fix(theming): fix TimeTable chart issues #34868
Conversation
|
/korbit-review |
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Inefficient Array Reversal ▹ view | ✅ Fix detected | |
| Redundant Type Conversions Per Render ▹ view | ✅ Fix detected | |
| Inefficient Array Length Access ▹ view | ✅ Fix detected | |
| Suboptimal Array Slice Usage ▹ view | ✅ Fix detected | |
| Memoize URL Template Rendering ▹ view | ✅ Fix detected | |
| Mixed presentation and data transformation concerns ▹ view | ✅ Fix detected | |
| Unsafe Type Assertion in MetricOption ▹ view | ✅ Fix detected | |
| Inefficient Double Iteration for Sum Calculation ▹ view | ✅ Fix detected | |
| Unsafe URL Template Rendering ▹ view | 🧠 Not in scope | |
| Incorrect Time Value Sorting ▹ view | ✅ Fix detected |
Files scanned
Due to the exceptionally large size of this PR, I've limited my review to the following files:
| File Path | Reviewed |
|---|---|
| superset-frontend/src/visualizations/TimeTable/components/LeftCell/index.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/validator/validateNonEmpty.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/Sparkline/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/ValueCell/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/SparklineCell/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/colorUtils/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sortUtils/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/constants.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/components/IconTooltip/index.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sparklineHelpers/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sparklineDataUtils/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/LeftCell/mustache.d.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/FormattedNumber.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/rowProcessing.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/index.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/types.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/Sparkline/Sparkline.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/ValueCell/ValueCell.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/colorUtils/colorUtils.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-nvd3/src/ReactNVD3.jsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sparklineDataUtils/sparklineDataUtils.ts | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/sparklineHelpers/sparklineHelpers.ts | ✅ |
| superset-frontend/src/explore/components/ControlHeader.tsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts | ✅ |
| superset-frontend/src/components/SQLEditorWithValidation/index.tsx | ✅ |
| superset-frontend/src/explore/components/controls/CollectionControl/index.jsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/TimeTable.tsx | ✅ |
| superset-frontend/src/explore/components/controls/OptionControls/index.tsx | ✅ |
| superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx | ✅ |
| superset-frontend/src/visualizations/TimeTable/components/SparklineCell/SparklineCell.tsx | ✅ |
| superset-frontend/src/explore/components/ControlPanelsContainer.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.
superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/rowProcessing.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/rowProcessing.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/FormattedNumber.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/FormattedNumber.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts
Outdated
Show resolved
Hide resolved
30871f0 to
887ccb9
Compare
887ccb9 to
ab7d74f
Compare
|
@sadpandajoe Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
| (Array.isArray(v) && v.length === 0) | ||
| ) { | ||
| return t('cannot be empty'); | ||
| return t('Cannot be empty'); |
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.
Did we confirm that this isn't in any translation files that need to be updated?
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.
You’re right, but I don’t think we should handle this here. I’ll revert the change, and we can address it later in a separate ticket along with the necessary translation updates.
|
@sadpandajoe Ephemeral environment spinning up at http://50.112.49.144:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
| body: { | ||
| overflow: 'hidden', | ||
| textOverflow: 'ellipsis', | ||
| textAlign: 'center', |
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.
Doesn't this affect all tooltips? Do we want everything to be center aligned?
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.
yup careful on touching core components as will ripple through the app...
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.
Sorry, my bad @sadpandajoe @mistercrunch. I was just testing something. I still think tooltip texts looks better center-aligned lol, but that’s not my call to make. Reverting this change
b04dabd to
8ae1898
Compare
|
@sadpandajoe Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@sadpandajoe Ephemeral environment spinning up at http://44.251.1.215:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
| mouseEnterDelay={0.3} | ||
| mouseLeaveDelay={0.15} |
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.
Do we not need these delays anymore?
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.
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.
|
🎪 Showtime deployed environment on GHA for 6281e5f • Environment: http://34.219.194.154:8080 (admin/admin) |
msyavuz
left a comment
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.
LGTM! Tested and works fine.
(cherry picked from commit d183969)
SUMMARY
This PR addresses theming and visual issues in Time-series charts while significantly improving the TimeTable component architecture through comprehensive refactoring:
Theming Fixes:
Component Structure Improvements:
BEFORE/AFTER SCREENSHOTS
Before:

After:

Before:

After:

Before:

After:

Before:

After:

Before:

After:

Before:

After:

TESTING INSTRUCTIONS
Time-series Tablechart (or create a new one)Time-series Period Pivotchart is also rendering as expected with themingADDITIONAL INFORMATION