You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LevisNgigi (via GitHub)" <gi...@apache.org> on 2025/04/02 09:44:06 UTC
[PR] feat(charts): add subheader option and metric customization controls [superset]
LevisNgigi opened a new pull request, #32975:
URL: https://github.com/apache/superset/pull/32975
feat(charts): add subheader option and metric customization controls
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR adds two key improvements to visualization controls:
1. Added subheader functionality to "Big Number with Trendline" and "Big Number with Time Comparisons" visualizations (the latter enabled via the CHART_PLUGINS_EXPERIMENTAL feature flag). This provides consistency with the standard Big Number visualization which already had this option.
2. Enhanced the explore view with controls that allow users to:
- Rename comparison metrics (main, #, and delta values)
- Toggle visibility of specific metrics
- Persist these customizations when the visualization is saved
These changes improve visualization customization options and provide a more consistent experience across related chart types.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. **Big Number with Trendline**:
- Create a new chart using the "Big Number with Trendline" visualization
- Verify the subheader option appears in the customize tab
- Add text to the subheader and confirm it displays correctly
2. **Big Number with Time Comparisons** (requires CHART_PLUGINS_EXPERIMENTAL flag):
- Enable the CHART_PLUGINS_EXPERIMENTAL feature flag
- Create a chart using the "Big Number with Time Comparisons" visualization
- Verify the subheader option appears and functions correctly
3. **Metric Customization**:
- Create a chart using "Table" visualizations and create a time comparison.
- In the explore view, locate the new controls for renaming comparison metrics
- Test renaming the main, #, and delta metrics
- Test toggling visibility of specific metrics
- Save the chart and verify the customizations persist when reopened
- Check that changes affect the visualization display correctly
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2792642290
@kgabryje Ephemeral environment spinning up at http://34.221.31.239:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2794126853
> @LevisNgigi Thanks! Unless @geido has more comments, I think we're good to go
@kgabryje Thank you for the feedback as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2790585427
@geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14364346617). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2786000703
@geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14331424569). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2790263238
> @LevisNgigi Thanks for the contribution, it's looking good! 'm not sure about is reverting to the metric name if the label is left empty and display type icon is unchecked. Also, if we use a saved metric with a label, the metric name displayed in time comparison when display type is unchecked is the metric's name (not "user friendly") instead of the label (see COUNT(*) metric displayed as count).
>
> 1 more thing - I think that the metric item is unnecessarily indented in the Customize columns control (see on the recording).
>
> Screen.Recording.2025-04-09.at.13.49.11.mov
@kgabryje About the not user friendly being displayed, I fixed that in my last commit such that if label is left empty(unchecked) and no custom column name,column name remains empty(as suggested in design) instead of displaying metric name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2035225530
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -873,6 +892,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
className,
value == null ? 'dt-is-null' : '',
isActiveFilterValue(key, value) ? ' dt-is-active-filter' : '',
+ 'test-c7w8t3',
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2789443918
@LevisNgigi Thanks for the contribution, it's looking good!
'm not sure about is reverting to the metric name if the label is left empty and display type icon is unchecked.
Also, if we use a saved metric with a label, the metric name displayed in time comparison when display type is unchecked is the metric's name (not "user friendly") instead of the label (see COUNT(*) metric displayed as count).
1 more thing - I think that the metric item is unnecessarily indented in the Customize columns control (see on the recording).
https://github.com/user-attachments/assets/7351d67e-07b2-4d9d-860c-fbb152645e90
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2035226076
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -795,6 +813,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
white-space: ${value instanceof Date ? 'nowrap' : undefined};
position: relative;
background: ${backgroundColor || undefined};
+ padding-left: ${column.isChildColumn ? '20px' : '8px'};
Review Comment:
Can we use theme.gridUnits here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2789448481
Oh and 1 more thing, not sure if related to your PR - the changes in Customize columns applied to the columns generated by time comparison (e.g. # count__1 month ago) have no effect on the table
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #32975:
URL: https://github.com/apache/superset/pull/32975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2031156343
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -229,6 +229,37 @@ class BigNumberVis extends PureComponent<BigNumberVizProps> {
return null;
}
+ renderSubtitle(maxHeight: number) {
+ const { subtitle, width } = this.props;
+ let fontSize = 0;
+
+ if (subtitle) {
+ const container = this.createTemporaryContainer();
+ document.body.append(container);
+ fontSize = computeMaxFontSize({
+ text: subtitle,
+ maxWidth: width * 0.9,
+ maxHeight,
+ className: 'subtitle-line',
+ container,
+ });
+ container.remove();
Review Comment:
makes sense agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2793813338
@kgabryje Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14383117121). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2035227768
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx:
##########
@@ -35,6 +36,57 @@ export default memo(function ColumnConfigItem({
}: ColumnConfigItemProps) {
const { colors, gridUnit } = useTheme();
const caretWidth = gridUnit * 6;
+
+ const outerContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ cursor: 'pointer',
+ padding: `${gridUnit}px ${2 * gridUnit}px`,
+ borderBottom: `1px solid ${colors.grayscale.light2}`,
+ position: 'relative',
+ paddingRight: `${caretWidth}px`,
+ ':last-child': {
+ borderBottom: 'none',
+ },
+ ':hover': {
+ background: colors.grayscale.light4,
+ },
+ '> .fa': {
+ color: colors.grayscale.light2,
+ },
+ ':hover > .fa': {
+ color: colors.grayscale.light1,
+ },
+ });
+
+ const nameContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ paddingLeft: column.isChildColumn ? gridUnit * 7 : gridUnit,
+ flex: 1,
+ });
+
+ const nameTextStyle = css({
+ paddingLeft: gridUnit,
+ });
+
+ const iconContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ position: 'absolute',
+ right: 3 * gridUnit,
+ top: 3 * gridUnit,
+ transform: 'translateY(-50%)',
+ gap: gridUnit,
+ color: colors.grayscale.light1,
+ });
+
+ const caretIconStyle = css({
+ fontSize: '14px',
Review Comment:
Can we use font sizes from theme?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx:
##########
@@ -35,6 +36,57 @@ export default memo(function ColumnConfigItem({
}: ColumnConfigItemProps) {
const { colors, gridUnit } = useTheme();
const caretWidth = gridUnit * 6;
+
+ const outerContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ cursor: 'pointer',
+ padding: `${gridUnit}px ${2 * gridUnit}px`,
+ borderBottom: `1px solid ${colors.grayscale.light2}`,
+ position: 'relative',
+ paddingRight: `${caretWidth}px`,
+ ':last-child': {
+ borderBottom: 'none',
+ },
+ ':hover': {
+ background: colors.grayscale.light4,
+ },
+ '> .fa': {
+ color: colors.grayscale.light2,
+ },
+ ':hover > .fa': {
+ color: colors.grayscale.light1,
+ },
+ });
+
+ const nameContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ paddingLeft: column.isChildColumn ? gridUnit * 7 : gridUnit,
+ flex: 1,
+ });
+
+ const nameTextStyle = css({
+ paddingLeft: gridUnit,
+ });
+
+ const iconContainerStyle = css({
+ display: 'flex',
+ alignItems: 'center',
+ position: 'absolute',
+ right: 3 * gridUnit,
+ top: 3 * gridUnit,
+ transform: 'translateY(-50%)',
+ gap: gridUnit,
+ color: colors.grayscale.light1,
+ });
+
+ const caretIconStyle = css({
+ fontSize: '14px',
Review Comment:
Can we use font size and font weight from theme?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2031122645
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -137,6 +139,21 @@ const colorPositiveNegative: ControlFormItemSpec<'Checkbox'> = {
debounceDelay: 200,
Review Comment:
### Inconsistent Debounce Delays <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple controls use different debounce delay values (200, 400, 500) without clear justification, which could lead to unnecessary re-renders.
###### Why this matters
Inconsistent debounce delays can cause UI jank and multiple re-renders when multiple controls are updated in quick succession.
###### Suggested change ∙ *Feature Preview*
Standardize debounce delays using constants and choose appropriate values based on the control type:
```typescript
const DEBOUNCE_DELAYS = {
IMMEDIATE: 50,
FAST: 200,
NORMAL: 400,
SLOW: 500,
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55196842-24cd-4cdc-929e-a6f8ee8200e0/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55196842-24cd-4cdc-929e-a6f8ee8200e0?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55196842-24cd-4cdc-929e-a6f8ee8200e0?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55196842-24cd-4cdc-929e-a6f8ee8200e0?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/55196842-24cd-4cdc-929e-a6f8ee8200e0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0afaf2c8-dccc-4f03-94fa-4bdef545833b -->
[](0afaf2c8-dccc-4f03-94fa-4bdef545833b)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx:
##########
@@ -140,6 +142,16 @@ export default function PopKPI(props: PopKPIProps) {
margin-bottom: ${theme.gridUnit * 4}px;
`;
+ const SubtitleText = styled.div`
+ ${({ theme }) => `
+ font-family: ${theme.typography.families.sansSerif};
+ font-weight: ${theme.typography.weights.medium};
+ text-align: center;
+ margin-top: -10px;
+ margin-bottom: ${theme.gridUnit * 4}px;
+ `}
+ `;
Review Comment:
### Unexplained Magic Number in Styles <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hardcoded magic number (-10px) in margin-top style without explanation or variable.
###### Why this matters
Magic numbers make it difficult to understand the reasoning behind specific values and complicate future maintenance.
###### Suggested change ∙ *Feature Preview*
Define the value as a themed constant or explain its purpose:
```typescript
const SUBTITLE_TOP_OFFSET = -10; // Offset to reduce gap between title and subtitle
const SubtitleText = styled.div`
${({ theme }) => `
margin-top: ${SUBTITLE_TOP_OFFSET}px;
...
`}
`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9c3ac5a-0945-4283-a85c-ce9ae4e06a67/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9c3ac5a-0945-4283-a85c-ce9ae4e06a67?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9c3ac5a-0945-4283-a85c-ce9ae4e06a67?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9c3ac5a-0945-4283-a85c-ce9ae4e06a67?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a9c3ac5a-0945-4283-a85c-ce9ae4e06a67)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:08b5a266-b2c5-49ed-bed0-33e40d51059f -->
[](08b5a266-b2c5-49ed-bed0-33e40d51059f)
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -229,6 +229,37 @@ class BigNumberVis extends PureComponent<BigNumberVizProps> {
return null;
}
+ renderSubtitle(maxHeight: number) {
+ const { subtitle, width } = this.props;
+ let fontSize = 0;
+
+ if (subtitle) {
+ const container = this.createTemporaryContainer();
+ document.body.append(container);
+ fontSize = computeMaxFontSize({
+ text: subtitle,
+ maxWidth: width * 0.9,
+ maxHeight,
+ className: 'subtitle-line',
+ container,
+ });
+ container.remove();
Review Comment:
### Unsafe DOM cleanup in renderSubtitle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The container.remove() operation could fail, leaving behind a DOM element if an error occurs during computeMaxFontSize.
###### Why this matters
A leaked DOM element could accumulate over time causing memory leaks and degraded performance.
###### Suggested change ∙ *Feature Preview*
Wrap the operation in a try-finally block to ensure cleanup:
```typescript
const container = this.createTemporaryContainer();
document.body.append(container);
try {
fontSize = computeMaxFontSize({
text: subtitle,
maxWidth: width * 0.9,
maxHeight,
className: 'subtitle-line',
container,
});
} finally {
container.remove();
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e985d5a-417b-4e13-9138-932e19a5c003/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e985d5a-417b-4e13-9138-932e19a5c003?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e985d5a-417b-4e13-9138-932e19a5c003?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e985d5a-417b-4e13-9138-932e19a5c003?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e985d5a-417b-4e13-9138-932e19a5c003)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:65aca0d8-85e1-4590-a64a-63449002d9ba -->
[](65aca0d8-85e1-4590-a64a-63449002d9ba)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2786023368
@geido Ephemeral environment spinning up at http://34.222.109.82:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2790614004
@geido Ephemeral environment spinning up at http://18.246.212.242:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2793282586
> > @kgabryje For number 2, I was to remove text Main from the main column as it does not stand out too much.
>
> Ahh I see so I suppose that empty cell is intentional? Maybe we could add the "time ago" suffix to the group header then for the distinction between the metric and the time comparison values, like `COUNT(*) 1 month ago`? And should we have a border between the metric header and `#`?
>
> <img alt="image" width="534" src="https://private-user-images.githubusercontent.com/15073128/432308785-cc118ba4-6d04-47bf-9a5c-a8334fa86770.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDQyOTMyNzAsIm5iZiI6MTc0NDI5Mjk3MCwicGF0aCI6Ii8xNTA3MzEyOC80MzIzMDg3ODUtY2MxMThiYTQtNmQwNC00N2JmLTlhNWMtYTgzMzRmYTg2NzcwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTA0MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwNDEwVDEzNDkzMFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTk1N2JiNTg5YmZiYTRjZTgxYjcxYThlYjliMjEwODNmODk1ZjNjMjBmM2I3ZDE3NGY2ZjNhNmExNWQ4OTFhZWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.ny2TNzF2J4fVq0xP39X3fZMQ5-uBdalYgSobK379FvU">
@kgabryje I have restored Main and all three issue previously mentioned were fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2795189123
> Debugging something funky that happening with this PR, somehow it failed when it merged on `master` and digging in the last commit's run here https://github.com/apache/superset/actions/runs/14382934496/job/40331066514, it appears that it skipped the python-related test, presumably because of an issue with our logic that checks which files have been touched by the PR and triggers/skips based on whether it sees the backend/frontend getting touched.
>
> I'm guessing the issue is around the logic checking for files touched by the PR, and specific to [maybe] having a merge-based workflow instead of a rebase-type workflow.
>
> Regardless, tests fail here and it's a bit tricky to sort this out, could use some help here to fix `master`
@mistercrunch noticed the error and saw it was a CI issue after checking the logs.Another check for a recently merged pr was running concurrently and not sure if it might have caused that?Something I can do to help?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subheader option and metric customization controls [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2772020571
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](7fea0700-5741-4396-9120-7170ad71bb4e)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2792599594
@kgabryje Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/14380586803). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2792853398
> Thanks for addressing the comments! I noticed 3 problems in the table chart:
>
> 1. There's an additional empty cell in the group header
> 2. Instead of "Main" we display the metric label
> 3. When I try to collapse the group, it disappears and the only way to restore it is to rerun the query or refresh the page
>
> I checked and those problems are not present on current master.
>
> Screen.Recording.2025-04-10.at.15.07.25.mov
@kgabryje For number 2, I was to remove text Main from the main column as it does not stand out too much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2035224415
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigControl.tsx:
##########
@@ -82,6 +82,9 @@ export default function ColumnConfigControl<T extends ColumnConfig>({
name: COLUMN_NAME_ALIASES[col] || col,
type: coltypes?.[idx],
config: value?.[col] || {},
+ isChildColumn: ['#', '△', '%'].some(
Review Comment:
I think this might break things if a regular column name starts with 1 of those symbols
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #32975:
URL: https://github.com/apache/superset/pull/32975#discussion_r2032905110
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx:
##########
@@ -73,18 +75,53 @@ export default memo(function ColumnConfigItem({
},
}}
>
- <ColumnTypeLabel type={column.type} />
- {column.name}
- {/* TODO: Remove fa-icon */}
- {/* eslint-disable-next-line icons/no-fa-icons-usage */}
- <i
- className="fa fa-caret-right"
+ <div
+ css={{
Review Comment:
Can we use the `css` with template literal approach for inline styles?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -368,7 +407,12 @@ export default styled(BigNumberVis)`
.subheader-line {
line-height: 1em;
- padding-bottom: 0;
+ padding-bottom: 0.3em;
+ }
+
+ .subtitle-line {
+ line-height: 1em;
+ padding-top: 0.3em;
Review Comment:
Can we indent these?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx:
##########
@@ -73,18 +75,53 @@ export default memo(function ColumnConfigItem({
},
}}
>
- <ColumnTypeLabel type={column.type} />
- {column.name}
- {/* TODO: Remove fa-icon */}
- {/* eslint-disable-next-line icons/no-fa-icons-usage */}
- <i
- className="fa fa-caret-right"
+ <div
+ css={{
+ display: 'flex',
+ alignItems: 'center',
+ paddingLeft: column.isChildColumn ? gridUnit * 7 : gridUnit,
+ flex: 1,
+ }}
+ >
+ <ColumnTypeLabel type={column.type} />
+ <span
+ css={{
+ paddingLeft: gridUnit,
+ }}
+ >
+ {column.name}
+ </span>
+ </div>
+
+ <div
css={{
+ display: 'flex',
+ alignItems: 'center',
position: 'absolute',
right: 3 * gridUnit,
top: 3 * gridUnit,
+ transform: 'translateY(-50%)',
+ gap: gridUnit,
+ color: colors.grayscale.light1,
}}
- />
+ >
+ {column.isChildColumn && column.config?.visible === false && (
+ <Icons.EyeInvisibleOutlined
+ css={{
+ fontWeight: 'bold',
+ fontSize: '14px',
+ color: colors.grayscale.base,
Review Comment:
We should use `iconSize` and `iconColor` props. I don't think we need to use bold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2792907431
> @kgabryje For number 2, I was to remove text Main from the main column as it does not stand out too much.
Ahh I see so I suppose that empty cell is intentional? Maybe we could add the "time ago" suffix to the group header then for the distinction between the metric and the time comparison values, like `COUNT(*) 1 month ago`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2793938429
@kgabryje Ephemeral environment spinning up at http://34.216.31.126:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2792795888
Thanks for addressing the comments!
I noticed 3 problems in the table chart:
1. There's an additional empty cell in the group header
2. Instead of "Main" we display the metric label
3. When I try to collapse the group, it disappears and the only way to restore it is to rerun the query or refresh the page
I checked and those problems are not present on current master.
https://github.com/user-attachments/assets/7ccc450b-16c2-4d32-9b1b-06356b2a4c73
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2795156311
Debugging something funky that happening with this PR, somehow it failed when it merged on `master` and digging in the last commit's run here https://github.com/apache/superset/actions/runs/14382934496/job/40331066514, it appears that it skipped the python-related test, presumably because of an issue with our logic that checks which files have been touched by the PR and triggers/skips based on whether it sees the backend/frontend getting touched.
I'm guessing the issue is around the logic checking for files touched by the PR, and specific to [maybe] having a merge-based workflow instead of a rebase-type workflow.
Regardless, tests fail here and it's a bit tricky to sort this out, could use some help here to fix `master`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): add subtitle option and metric customization controls [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #32975:
URL: https://github.com/apache/superset/pull/32975#issuecomment-2793712541
@LevisNgigi Thanks! Unless @geido has more comments, I think we're good to go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org